feat: Embedded SDK

[suddjian]

SUMMARY

Part of #17187.

Implements the frontend sdk that will be used by host apps to embed dashboards.

See the included documentation files for more info on how this package works.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

This will be easier to test once we add a demo app that uses it. For the time being if you want to test this you'd have to try the feature out in your own app. The added README.md file explains how to do this.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[michael-s-molina]

I'm not sure about this. It would be really cool if we could spend more time trying to use Jest/JSDOM to write tests that check for specific conditions like:

• Failure when invoking fetchGuestToken
• An unknown id
• An invalid supersetDomain
• Test the unmount behavior

You could use "testEnvironmentOptions": { "resources": "usable" } in jest.config.js to enable sub-resouces like iframes.

[suddjian]

Hmm, I think you're right that it would be good to unit test the fetchGuestToken handling. And it would be nice to at least have one test to ensure that the code doesn't blow up. As for the others though, in my experience, unit testing helps most when you have branching logic in your code, and this code currently doesn't contain even one if statement. It just executes a series of instructions, all of which are interactions with external APIs. So, unit testing it beyond the "it doesn't blow up" case will essentially be asserting that the code exists as written. Those kinds of tests tend to just get in the way of changing the code instead of really helping much.

I'll also add an "on pull request" github action that makes sure the build can still run.

[michael-s-molina]

I think there are valid test cases for this code and some of the test cases can even introduce if or try/catch blocks in the logic. Some examples:

If a user passes an unknown dashboard id like one from a deleted dashboard, what happens when this piece of code executes?

iframe.src = `${supersetDomain}/dashboard/${id}/embedded`;

If the user passes an invalid supersetDomain what happens when this piece of code executes?

iframe.contentWindow!.postMessage(
  { type: IFRAME_COMMS_MESSAGE_TYPE, handshake: "port transfer" },
  supersetDomain,
  [theirPort],
)

For both scenarios, are we going to display the default browser error or a custom message inside the iframe? if we chose to display a custom message, the test cases could account for that.

[suddjian]

For case 1, the SDK does not have any clue that the id doesn't exist. All it does is create an iframe and send a guest token to it. If we want to display some special message in that case, that would happen on the superset side, not in the SDK.

In the second case, what happens is entirely dependent on browser implementation. Some browsers could throw an error, which would be fine imo since the caller passed in the wrong thing it should probably throw anyway. But some browsers silently ignore the message, I believe this is to prevent a malicious page from gathering info about other pages.

[michael-s-molina]

In the case a browser silently ignores the message, won't this line fail later?

ourPort.postMessage({ guestToken });

[suddjian]

It will fail to actually convey the guest token to its intended recipient since no one is listening on the other side, but it won't throw an error. The message channel and port do exist, they're just not connected to anything.

[suddjian]

I wrote a test for "it doesn't blow up", but actually jsdom doesn't seem to support the iframe's sandbox property. It's just not present on the iframe element. The jsdom changelog says that it was removed many versions ago.

Unless someone knows of a good way around that, I think I'll forego unit testing in favor of thorough cypress testing using the demo host app, which should be added soon. That way we can make actual assertions about the behaviors and security characteristics of this system. For now, this code has been manually tested pretty thoroughly in a couple of applications, and we're focused on getting it usable asap.

[suddjian]

Hopefully this workflow doesn't have any issues, or I'll need a follow-up PR to fix it. Reviewers please help me check this! 🙏

[suddjian]

I tested and got it working on my fork, so I can be pretty confident that it will work here.

[villebro]

A few comments relating to GHA workflows

[villebro]

In other similar flows we have slightly more specific ons to avoid CI running on Draft PRs. Something like this perhaps?

on:
  push:
    branches:
      - 'master'
  pull_request:
    paths:
      - "superset-embedded-sdk/**"
    types: [synchronize, opened, reopened, ready_for_review]

[suddjian]

nice, thanks!

[villebro]

Any need to add node_modules or other typical cruft here?

[suddjian]

nah, the root .gitignore file also applies here