Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Embedding SDK - integration commit #40198

Merged
merged 14 commits into from
Mar 26, 2024
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
/resources/frontend_client/embed.html
/resources/frontend_client/index.html
/resources/frontend_client/public.html
/resources/embedding-sdk/dist/
/resources/i18n/*.edn
/resources/instance_analytics.zip
/resources/license-backend-third-party.txt
Expand Down
8 changes: 8 additions & 0 deletions enterprise/frontend/src/embedding-sdk/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# Embedding SDK

### Build
`yarn build`

`yarn build-embedding-sdk`

Build results are located at `<root>/resources/embedding-sdk`
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import styled from "@emotion/styled";

import { alpha, color } from "metabase/lib/colors";
import { aceEditorStyles } from "metabase/query_builder/components/NativeQueryEditor/NativeQueryEditor.styled";
import { saveDomImageStyles } from "metabase/visualizations/lib/save-chart-image";

export const SdkContentWrapper = styled.div<{ font: string }>`
--default-font-family: "${({ font }) => font}";
--color-brand: ${color("brand")};
--color-brand-alpha-04: ${alpha("brand", 0.04)};
--color-brand-alpha-88: ${alpha("brand", 0.88)};
--color-focus: ${color("focus")};

${aceEditorStyles}
${saveDomImageStyles}

--default-font-size: 0.875em;
--default-font-color: var(--color-text-dark);
--default-bg-color: var(--color-bg-light);

font-family: var(--default-font-family), sans-serif;
font-size: var(--default-font-size);
font-weight: 400;
font-style: normal;
color: var(--color-text-dark);
margin: 0;
height: 100%; /* ensure the entire page will fill the window */
display: flex;
flex-direction: column;
background-color: var(--color-bg-light);

-webkit-font-smoothing: antialiased;
-moz-osx-font-smoothing: grayscale;
`;
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import type * as React from "react";
import { memo, useEffect, useState } from "react";
import { Provider } from "react-redux";

import reducers from "metabase/reducers-main";
import { setOptions } from "metabase/redux/embed";
import { getStore } from "metabase/store";
import { EmotionCacheProvider } from "metabase/styled-components/components/EmotionCacheProvider";
import { ThemeProvider } from "metabase/ui/components/theme/ThemeProvider";

import type { SDKConfigType } from "../../config";
import { EmbeddingContext } from "../../context";
import { useInitData } from "../../hooks";
import { SdkContentWrapper } from "../SdkContentWrapper";

import "metabase/css/vendor.css";
import "../../styles.css";

const MetabaseProviderInternal = ({
children,
config,
}: {
children: React.ReactNode;
config: SDKConfigType;
}): JSX.Element => {
const store = getStore(reducers);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I moved store initialization out of the component


const [font, setFont] = useState<string>(config.font ?? "Lato");

useEffect(() => {
if (font) {
store.dispatch(setOptions({ font }));
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [font]);

const { isLoggedIn, isInitialized } = useInitData({
store,
config,
});

return (
<EmbeddingContext.Provider
value={{
isInitialized,
isLoggedIn,
font,
setFont,
}}
>
<Provider store={store}>
<EmotionCacheProvider>
<ThemeProvider>
<SdkContentWrapper font={font}>
{!isInitialized ? <div>Initializing...</div> : children}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be wrapped in t?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

</SdkContentWrapper>
</ThemeProvider>
</EmotionCacheProvider>
</Provider>
</EmbeddingContext.Provider>
);
};

export const MetabaseProvider = memo(MetabaseProviderInternal);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@deniskaber deniskaber Mar 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
import { useEffect, useState } from "react";

import LoadingAndErrorWrapper from "metabase/components/LoadingAndErrorWrapper";
import { GET, POST } from "metabase/lib/api";
import { useSelector } from "metabase/lib/redux";
import {
onCloseChartType,
onOpenChartSettings,
setUIControls,
} from "metabase/query_builder/actions";
import QueryVisualization from "metabase/query_builder/components/QueryVisualization";
import ChartTypeSidebar from "metabase/query_builder/components/view/sidebars/ChartTypeSidebar";
import { getMetadata } from "metabase/selectors/metadata";
import { Box, Button, Group, Text } from "metabase/ui";
import { PublicMode } from "metabase/visualizations/click-actions/modes/PublicMode";
import Question from "metabase-lib/v1/Question";
import type { Card, CardId, Dataset } from "metabase-types/api";

import { useEmbeddingContext } from "../../hooks";

interface QueryVisualizationProps {
questionId: CardId;
showVisualizationSelector?: boolean;
}

type State = {
loading: boolean;
card: Card | null;
result: Dataset | null;
};

export const QueryVisualizationSdk = (
props: QueryVisualizationProps,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we destructure the props here like we do with other components for consistency? This will also make sure any prop destructured here will definitely be used because it will be caught by ESLint otherwise. This could help prevent the case where we have more properties in QueryVisualizationProps but don't use all of them.

): JSX.Element | null => {
const { isInitialized, isLoggedIn } = useEmbeddingContext();
const metadata = useSelector(getMetadata);

const { questionId } = props;

const [state, setState] = useState<State>({
loading: false,
card: null,
result: null,
});

const loadCardData = async ({ questionId }: { questionId: number }) => {
setState(prevState => ({
WiNloSt marked this conversation as resolved.
Show resolved Hide resolved
...prevState,
loading: true,
}));

Promise.all([
GET("/api/card/:cardId")({ cardId: questionId }),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you create services files to host all the endpoints used in the SDK? This will help us know which endpoints are being used in the SDK which might help with the API versioning.

example.

get: GET("/api/card/:cardId"),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will use current CardApi object here!

POST("/api/card/:cardId/query")({
cardId: questionId,
}),
])
.then(([card, result]) => {
setState(prevState => ({
...prevState,
card,
result,
loading: false,
}));
})
.catch(([_cardError, resultError]) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't you set cardError to error the same we you did with result?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

setState(prevState => ({
...prevState,
result: resultError?.data,
}));
});
};

useEffect(() => {
if (!isInitialized || !isLoggedIn) {
setState({
loading: false,
card: null,
result: null,
});

return;
}

loadCardData({ questionId });
}, [isInitialized, isLoggedIn, questionId]);

const { card, result } = state;

const changeVisualization = (newQuestion: Question) => {
setState({
card: newQuestion.card(),
result: state.result,
loading: false,
});
};

if (!isInitialized) {
return null;
}

if (!isLoggedIn) {
return (
<div>
<Text>You should be logged in to see this content.</Text>
<Button>Log in</Button>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a dummy login button? It doesn't look like it will initiate the login action since it's not wired to anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes. I was added for a demo 👍 I will remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plan is to refactor this part when we have design for a case of SDK components usage for unauthenticated user.

</div>
);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

error={typeof result === "string" ? result : null}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like like it should be more obvious if there is an error rather than having to check the type of the property. What about having other keys cardError andresultError, or changing result to be an object with 2 properties

  • data and
  • error

This way it should be very obvious if there's an error e.g.

error={resultError}
// or
error={result.error}

I think typeof result === "string" is ambiguous and doesn't convey that it has an error. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All logic to handle data and errors was copied from https://github.com/metabase/metabase/blob/master/frontend/src/metabase/public/containers/PublicQuestion/PublicQuestion.jsx#L226

I think it's good to refactor this, but I am bit hesitant to do this in scope of this PR. I agree that this code is far from ideal, but I would rather move this improvements to a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, added resultError and cardError

noWrapper
>
{() => {
const question = new Question(card, metadata);
const legacyQuery = question.legacyQuery({
useStructuredQuery: true,
});

return (
<Group h="100%" pos="relative" align="flex-start">
{props.showVisualizationSelector && (
<Box w="355px">
<ChartTypeSidebar
question={question}
result={result}
onOpenChartSettings={onOpenChartSettings}
onCloseChartType={onCloseChartType}
query={legacyQuery}
setUIControls={setUIControls}
updateQuestion={changeVisualization}
/>
</Box>
)}
<QueryVisualization
className="flex full-width"
question={question}
rawSeries={[{ card, data: result && result.data }]}
isRunning={state.loading}
isObjectDetail={false}
isResultDirty={false}
isNativeEditorOpen={false}
result={result}
noHeader
mode={PublicMode}
/>
</Group>
);
}}
</LoadingAndErrorWrapper>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { QueryVisualizationSdk } from "./QueryVisualization";
export { MetabaseProvider } from "./MetabaseProvider";
6 changes: 6 additions & 0 deletions enterprise/frontend/src/embedding-sdk/config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export type SDKConfigType = {
metabaseInstanceUrl: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

font?: string;
authType: "apiKey";
apiKey: string;
};
19 changes: 19 additions & 0 deletions enterprise/frontend/src/embedding-sdk/context.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { createContext, useContext } from "react";

interface EmbeddingSdkContextData {
isInitialized: boolean;
isLoggedIn: boolean;
font: string | null;
setFont: (font: string) => void;
}

export const EmbeddingContext = createContext<EmbeddingSdkContextData>({
isInitialized: false,
isLoggedIn: false,
font: null,
setFont: () => {},
});

export const useEmbeddingContext = () => {
return useContext(EmbeddingContext);
};
2 changes: 2 additions & 0 deletions enterprise/frontend/src/embedding-sdk/hooks/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export * from "./public";
export * from "./private";
2 changes: 2 additions & 0 deletions enterprise/frontend/src/embedding-sdk/hooks/private/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export * from "./use-sdk-context";
export * from "./use-init-data";
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import { useEffect, useState } from "react";
import _ from "underscore";

import { reloadSettings } from "metabase/admin/settings/settings";
import api from "metabase/lib/api";
import { refreshCurrentUser } from "metabase/redux/user";
import registerVisualizations from "metabase/visualizations/register";

import type { SDKConfigType } from "../../config";

const registerVisualizationsOnce = _.once(registerVisualizations);

type InitDataLoaderProps = {
store: any;
config: SDKConfigType;
};

export const useInitData = ({
store,
config,
}: InitDataLoaderProps): {
isLoggedIn: boolean;
isInitialized: boolean;
} => {
const [isInitialized, setIsInitialized] = useState(false);
const [isLoggedIn, setIsLoggedIn] = useState(false);

useEffect(() => {
registerVisualizationsOnce();
}, []);

useEffect(() => {
api.basename = config.metabaseInstanceUrl;

if (config.authType === "apiKey" && config.apiKey) {
api.apiKey = config.apiKey;
} else {
setIsLoggedIn(false);
return;
}

Promise.all([
store.dispatch(refreshCurrentUser()),
store.dispatch(reloadSettings()),
]).then(() => {
setIsInitialized(true);
setIsLoggedIn(true);
Comment on lines +46 to +47
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@deniskaber deniskaber Mar 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

});
}, [config, store]);

return {
isLoggedIn,
isInitialized,
};
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { useContext } from "react";

import { EmbeddingContext } from "../../context";

export const useEmbeddingContext = () => {
return useContext(EmbeddingContext);
};
4 changes: 4 additions & 0 deletions enterprise/frontend/src/embedding-sdk/hooks/public/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export * from "./use-current-user";
export * from "./use-application-name";
export * from "./use-question-search";
export * from "./use-available-fonts";
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import { useSelector } from "metabase/lib/redux";
import { getApplicationName } from "metabase/selectors/whitelabel";

export const useApplicationName = () => useSelector(getApplicationName);
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { useSelector } from "metabase/lib/redux";
import { getSetting } from "metabase/selectors/settings";

import { useEmbeddingContext } from "../../hooks/private/use-sdk-context";

export const useAvailableFonts = () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see we export setFont here (used here). Should we? This is another reason why I mentioned earlier why I think we should have at least some simple documents how to use each components in the SDK, so we know how they're intended for consumption.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remove this usage from embedding-sdk-customer-zero, but forgot to push it there 😄 . Pushed now

const { font, setFont } = useEmbeddingContext();

return {
availableFonts: useSelector(state => getSetting(state, "available-fonts")),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currentFont: font,
setFont,
};
};
Loading
Loading