Embedding SDK - integration commit #40198

deniskaber · 2024-03-15T19:10:26Z

Relates to #40241

Description

Embedding SDK code.

How to verify

Demo

Checklist

Tests have been added/updated to cover changes in this PR

github-actions · 2024-03-15T19:12:03Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 4543a7f...21428df.

Notify	File(s)
@alxnddr	frontend/src/metabase/visualizations/components/ChoroplethMap.jsx

replay-io · 2024-03-15T19:23:14Z

Status	Complete ↗︎
Commit	`21428df`
Results	⚠️ 6 Flaky ✅ 2378 Passed

npretto · 2024-03-22T12:51:21Z

enterprise/frontend/src/embedding-sdk/components/public/MetabaseProvider.tsx

+  children: React.ReactNode;
+  config: SDKConfigType;
+}): JSX.Element => {
+  const store = getStore(reducers);


I know this component is wrapped in memo, but isn't it a bit risky to create the store on each "render"?
Docs says "But React may still re-render it: memoization is a performance optimization, not a guarantee."

Good point. I moved store initialization out of the component

npretto · 2024-03-22T12:56:04Z

enterprise/frontend/src/embedding-sdk/components/public/MetabaseProvider.tsx

+        <EmotionCacheProvider>
+          <ThemeProvider>
+            <SdkContentWrapper font={font}>
+              {!isInitialized ? <div>Initializing...</div> : children}


Should this be wrapped in t?

npretto · 2024-03-22T13:03:59Z

enterprise/frontend/src/embedding-sdk/components/public/QueryVisualization.tsx

+  return (
+    <LoadingAndErrorWrapper
+      className="flex-full full-width"
+      loading={!result}


We're using loading={!result} here but isRunning={state.loading} later on, should we use state.loading for both?

It was copy pasted from the original QueryVisualization component, but it makes sense to refactor this a bit.
Added a fix

npretto · 2024-03-22T13:44:26Z

enterprise/frontend/src/embedding-sdk/config.ts

@@ -0,0 +1,6 @@
+export type SDKConfigType = {
+  metabaseInstanceUrl: string;


nit: should we just call it "instanceUrl" to help with white-labeling customers?
There's a slack thread about that

In the thread I see that we don't really care about source code part.
Here it will also make it harder for developers to use the SDK.

metabaseInstanceUrl is more readable than instanceUrl. Developer who will configure this would have to think which value to put here.

npretto · 2024-03-22T13:48:43Z

enterprise/frontend/src/embedding-sdk/hooks/public/use-available-fonts.ts

+  const { font, setFont } = useEmbeddingContext();
+
+  return {
+    availableFonts: useSelector(state => getSetting(state, "available-fonts")),


I think this is the first time I see a hook inside a return 😮 , it makes sense though

WiNloSt

I haven't reviewed the webpack config. I think Uladzimir work with that a lot, it could be great to have him review it as well.

WiNloSt · 2024-03-25T07:11:41Z

frontend/src/metabase/lib/api.js

@@ -89,6 +99,15 @@ export class Api extends EventEmitter {
          delete headers["Content-Type"];
        }

+        if (this.apiKey) {
+          headers["x-api-key"] = this.apiKey;


nit: Just so the header name follows the standard naming convention.

Suggested change

headers["x-api-key"] = this.apiKey;

headers["X-Api-Key"] = this.apiKey;

WiNloSt · 2024-03-25T07:22:51Z

enterprise/frontend/src/embedding-sdk/hooks/private/use-init-data.ts

+      setIsInitialized(true);
+      setIsLoggedIn(true);


would it be possible that either refreshCurrentUser or reloadSettings would fail? i.e. if it returns 401 or something because the token is wrong., should the promise still be resolved? It just feels like we should check the result from calling these endpoints before setting isLoggedIn to true just to make sure we can call these endpoints.

What do you think?

Here we have Promise.all. API calls inside refreshCurrentUser and reloadSettings have awaits, so in case any call fails, we would not get to .then.

We should add some error handling for sure, something like an ErrorBoundary in QueryVisualizationSdk component, but I don't think this fits in this PR scope.

Alright, I found that we reject anything not within 2xx status, so this is good. I was worried errored requests are still going through .then

metabase/frontend/src/metabase/lib/api.js

Lines 202 to 212 in dc279fd

if (status >= 200 && status <= 299) {

if (options.transformResponse) {

body = options.transformResponse(body, { data });

}

resolve(body);

} else {

reject({

status: status,

data: body,

isCancelled: isCancelled,

});

WiNloSt · 2024-03-25T07:26:20Z

enterprise/frontend/src/embedding-sdk/components/private/AppInitializeController.tsx

+
+  useEffect(() => {
+    if (font) {
+      dispatch(setOptions({ font }));


Are we setting any option other than font in SDK right now? I'm asking because setOptions overrides all the embed config. If we set it to something else else where and change font, other settings will be reverted to default values.

Now we don't set any options other than font. But that's a valid concern for sure! I will add a comment to address in future.

WiNloSt · 2024-03-25T07:27:19Z

enterprise/frontend/src/embedding-sdk/components/private/AppInitializeController.tsx

+        isLoggedIn,
+      }}
+    >
+      <SdkContentWrapper data-elementid="sdk-content-wrapper" font={font}>


What is data-elementid for? I couldn't find its reference in the app.

I kind of get we pass font here because it's used in SdkContentWrapper, but why do we need to set it in setOptions too? Isn't setting font in SdkContentWrapper enough?

Good point! We might don't need to have it in our state as we don't use GlobalStyles component from the main app, which is connected to redux state.
In our case it is passed to us from the outside, so it's not logical to have it exposed from useAvailableFonts hook - users already know which value they use.

And what is data-elementid for?

It's just a way for us to detect this Global styles component on a page.
We can remove it for now.

WiNloSt · 2024-03-25T07:44:57Z

enterprise/frontend/src/embedding-sdk/components/public/MetabaseProvider.tsx

+  );
+};
+
+export const MetabaseProvider = memo(MetabaseProviderInternal);


It'd be nice, if you include an example how this provider is supposed to be used in enterprise/frontend/src/embedding-sdk/README.md. Now that readme file is pretty barebone.

It's hard to just imagine how this component is supposed to be used without context.

Otherwise, we could have a long comment above this file documenting how it's supposed to be used.

Example on how to use SDK usage would be self-described in the host app example - https://github.com/metabase/embedding-sdk-customer-zero/blob/pre-alpha/src/App.js#L20 .

we will have types for public components added in the next task.

But I will add an action item to add documentation for the SDK at some point.

WiNloSt · 2024-03-25T08:59:04Z

frontend/src/metabase/lib/api.js


  GET;
  POST;
  PUT;
  DELETE;

-  constructor() {
+  constructor(basename) {


Suggested change

constructor(basename) {

constructor() {

You could remove line 50-51 as well.

I don't think you pass anything to the constructor right now.

metabase/frontend/src/metabase/lib/api.js

Line 311 in 504840b

const instance = new Api();

This is being set to the exported object directly here.

metabase/enterprise/frontend/src/embedding-sdk/hooks/private/use-init-data.ts

Line 36 in 504840b

api.apiKey = config.apiKey;

frontend/src/metabase/lib/api.js

WiNloSt · 2024-03-25T09:02:54Z

frontend/src/metabase/redux/settings.ts

@@ -20,7 +20,7 @@ export const refreshSiteSettings = createAsyncThunk(
 );

 export const settings = createReducer(
-  { values: window.MetabaseBootstrap, loading: false },
+  { values: window.MetabaseBootstrap || {}, loading: false },


Why is this needed? Have some types changed?

In our SDK initialization logic, we don't have window.MetabaseBootstrap available.
In the regular app, BE injects this data into generated html using this script in this html template. There were some errors because we have undefined instead of an object. So, we can either add a default value to the global window object, or add it here. In my opinion the latter is a bit cleaner

WiNloSt · 2024-03-25T09:02:58Z

frontend/src/metabase/visualizations/components/ChoroplethMap.jsx

@@ -153,7 +153,7 @@ export default class ChoroplethMap extends Component {
          this.setState({
            geoJson: geoJson,
            geoJsonPath: geoJsonPath,
-            minimalBounds: computeMinimalBounds(geoJson.features),
+            minimalBounds: computeMinimalBounds(geoJson?.features ?? []),


Why is this needed? Have some types changed?

We had an issue that this line was empty in our case. We might be missing some predefined state or initialization steps, but with this change it seems to be working as it should be

WiNloSt · 2024-03-25T09:05:50Z

tsconfig.sdk.json

+  "exclude": [
+    "node_modules",
+    // The following files will load `frontend/src/metabase/app` which will conflict with
+    // `frontend/src/metabase/App.tsx`, since now we're loading both JS and TS files.


We probably should do something about this (not in this PR though). VS Code confuses frontend/src/metabase/App.tsx with frontend/src/metabase/app.js and that's frustrated when working with these files.

maybe renaming app.js to init.js could help, as it seems it's just code to init the app

yeah, that would do. We don't have init.* in frontend/src/metabase/ currently.

This has been copied from the regular https://github.com/metabase/metabase/blob/master/tsconfig.json#L23 used in the main app.

Yeah, I know that because I wrote that comment 😅 . It's just that this is something we should be aware of at least because I'm not particularly satisfied with the solution I went with either.

WiNloSt

This is already a lot better, but I'd like more people to have a look at the webpack config.

WiNloSt · 2024-03-26T07:47:03Z

enterprise/frontend/src/embedding-sdk/components/public/QueryVisualization.tsx

+        cardError: null,
+        resultError: null,
+      });
+    } else {


Oh, I like the else part more compared to the previous solution. I was thinking something felt off, but I couldn't think of anything else at that time.

WiNloSt · 2024-03-26T07:54:23Z

enterprise/frontend/src/embedding-sdk/hooks/private/use-init-data.ts

+      setIsInitialized(true);
+      setIsLoggedIn(true);


Alright, I found that we reject anything not within 2xx status, so this is good. I was worried errored requests are still going through .then

metabase/frontend/src/metabase/lib/api.js

Lines 202 to 212 in dc279fd

if (status >= 200 && status <= 299) {

if (options.transformResponse) {

body = options.transformResponse(body, { data });

}

resolve(body);

} else {

reject({

status: status,

data: body,

isCancelled: isCancelled,

});

WiNloSt · 2024-03-26T07:56:11Z

enterprise/frontend/src/embedding-sdk/components/private/AppInitializeController.tsx

+        isLoggedIn,
+      }}
+    >
+      <SdkContentWrapper data-elementid="sdk-content-wrapper" font={font}>


And what is data-elementid for?

uladzimirdev

Added some minor comments re webpack config. It's very similar to our main webpack config.

Overall comments:

webpack config can be reused, we really override just a few lines, so no need to copy everything, especially it will become a nightmare in support
nit. It looks like we can keep package.json.template with empty dependencies if the idea is to use it only for publishing and remove .template in the pre-publish step. It can potentially save us from unnecessary node_modules installation.

uladzimirdev · 2024-03-26T16:19:53Z

webpack.embedding-sdk.config.js

+            {
+              loader: "style-loader",
+            },
+            //   { loader: MiniCssExtractPlugin.loader },


I think I tried it in a main webpack config and it broke half of the styles

uladzimirdev · 2024-03-26T16:26:45Z

webpack.embedding-sdk.config.js

+      path: BUILD_PATH + "/dist",
+      publicPath: "",
+      filename: "[name].bundle.js",
+      libraryTarget: "commonjs2",


https://webpack.js.org/configuration/output/#type-commonjs2

output: { library: { // note there's no `name` here type: 'commonjs2', }, },

uladzimirdev · 2024-03-26T16:32:03Z

webpack.embedding-sdk.config.js

+      new webpack.ProvidePlugin({
+        process: "process/browser.js",
+      }),
+      new ForkTsCheckerWebpackPlugin({


I don't see a reason to use it instead of normal tsc at CI, but if you have arguments 👍

We will use it to generate .d.ts files for SDK bundle later on

isn't tsc enough for it?

github-actions · 2024-03-26T23:10:51Z

@deniskaber Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

deniskaber self-assigned this Mar 15, 2024

deniskaber requested a review from camsaul as a code owner March 15, 2024 19:10

deniskaber added no-backport Do not backport this PR to any branch .Team/Embedding labels Mar 15, 2024

deniskaber changed the title ~~Embedding SDK - initial commit~~ Embedding SDK - integration commit Mar 15, 2024

metabase-bot bot added the visual Run Percy visual testing label Mar 15, 2024

This was referenced Mar 18, 2024

[Epic] Embedding SDK PoC #39115

Closed

[Epic] Embedding SDK – Finalize build #40241

Closed

deniskaber force-pushed the embedding-sdk-initial branch from 7657e1b to 2bfd09e Compare March 18, 2024 15:53

deniskaber removed the request for review from camsaul March 18, 2024 19:43

deniskaber added 4 commits March 21, 2024 18:48

Minimal SDK code

f48ca34

Fix viz, styles

badc186

Move SDK code to enterprise folder

900b76e

Fix files structure

ebd4ef2

deniskaber force-pushed the embedding-sdk-initial branch from 6a3712b to ebd4ef2 Compare March 21, 2024 15:49

Clean-up

e75fad0

deniskaber requested a review from a team March 21, 2024 19:07

npretto reviewed Mar 22, 2024

View reviewed changes

npretto requested a review from a team March 22, 2024 12:52

npretto reviewed Mar 22, 2024

View reviewed changes

Review fixes

504840b

deniskaber requested review from npretto and a team March 22, 2024 16:44

WiNloSt reviewed Mar 25, 2024

View reviewed changes

deniskaber requested a review from uladzimirdev March 25, 2024 12:34

deniskaber added 2 commits March 25, 2024 18:20

Review fixes

36d9212

Review fixes

54ce503

deniskaber requested a review from WiNloSt March 25, 2024 18:11

deniskaber requested a review from a team March 25, 2024 18:11

WiNloSt approved these changes Mar 26, 2024

View reviewed changes

Remove elementid

152c30b

uladzimirdev approved these changes Mar 26, 2024

View reviewed changes

deniskaber added 4 commits March 26, 2024 22:39

Reuse some options from main webpack config

f1e3055

Actualize package.json

2e79312

Merge branch 'master' into embedding-sdk-initial

8f8d7a7

Actualize package.json

16e871b

deniskaber enabled auto-merge (squash) March 26, 2024 20:39

Revert embed reducer changes

21428df

deniskaber merged commit 98f6cd8 into master Mar 26, 2024
109 checks passed

deniskaber deleted the embedding-sdk-initial branch March 26, 2024 23:10

deniskaber added this to the 0.50 milestone Mar 26, 2024

		@@ -0,0 +1,6 @@
		export type SDKConfigType = {
		metabaseInstanceUrl: string;

	headers["x-api-key"] = this.apiKey;
	headers["X-Api-Key"] = this.apiKey;

	if (status >= 200 && status <= 299) {
	if (options.transformResponse) {
	body = options.transformResponse(body, { data });
	}
	resolve(body);
	} else {
	reject({
	status: status,
	data: body,
	isCancelled: isCancelled,
	});

Embedding SDK - integration commit #40198

Embedding SDK - integration commit #40198

Conversation

deniskaber commented Mar 15, 2024 • edited

Description

How to verify

Demo

Checklist

github-actions bot commented Mar 15, 2024 • edited

replay-io bot commented Mar 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WiNloSt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deniskaber Mar 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deniskaber Mar 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WiNloSt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uladzimirdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 26, 2024

deniskaber commented Mar 15, 2024 •

edited

github-actions bot commented Mar 15, 2024 •

edited

replay-io bot commented Mar 15, 2024 •

edited

deniskaber Mar 25, 2024 •

edited

deniskaber Mar 25, 2024 •

edited