Move init to promise #152
Move init to promise #152
Conversation
commit: |
src/embed/base.ts
Outdated
| const isInitCalledKey = 'isInitCalled'; | ||
|
|
||
| const createAndSetInitPromise = (): void => { | ||
| const initPromise = new Promise<ReturnType<typeof init>>((resolve) => { |
There was a problem hiding this comment.
There was a problem hiding this comment.
I can upgarde ts libs with es2024 to use this but this is quite new .
There was a problem hiding this comment.
Its in the baseline so supported in all browsers since 1 year, is there a concern ?
src/embed/base.ts
Outdated
| const initPromise = new Promise<ReturnType<typeof init>>((resolve) => { | ||
| storeValueInWindow(initPromiseResolveKey, resolve); | ||
| }); | ||
| storeValueInWindow(isInitCalledKey, false); |
There was a problem hiding this comment.
can we store a single value in the window ? Store an object with these 3 properties
src/embed/ts-embed.ts
Outdated
| * @returns | ||
| */ | ||
| public async prerenderGeneric(): Promise<any> { | ||
| await this.isReadyForRender; |
There was a problem hiding this comment.
isn;t readyForRender a boolean ?
| */ | ||
| public async render(): Promise<TsEmbed> { | ||
| if (!getIsInitCalled()) { | ||
| logger.error(ERROR_MESSAGE.RENDER_CALLED_BEFORE_INIT); |
There was a problem hiding this comment.
Cannot use warn here ,
If you have never done init , then loglevel by default is error. So warns will not be shown by logger.
There was a problem hiding this comment.
We should have loglevel by default to warn ?
src/embed/ts-embed.ts
Outdated
| let readyForRenderResolve: (value?: boolean) => void; | ||
| let readyForRenderReject: (reason?: any) => void; | ||
|
|
||
| this.isReadyForRenderPromise = new Promise<boolean>((resolve, reject) => { | ||
| readyForRenderResolve = resolve; | ||
| readyForRenderReject = reject; | ||
| }); | ||
|
|
||
| getInitPromise().then(async () => { | ||
| this.isReadyForRender = true; | ||
| // TODO: handle error | ||
| const embedConfig = getEmbedConfig(); | ||
| this.embedConfig = embedConfig; | ||
| if (!embedConfig.authTriggerContainer && !embedConfig.useEventForSAMLPopup) { | ||
| this.embedConfig.authTriggerContainer = domSelector; | ||
| } | ||
| this.thoughtSpotHost = getThoughtSpotHost(embedConfig); | ||
| this.thoughtSpotV2Base = getV2BasePath(embedConfig); | ||
| this.shouldEncodeUrlQueryParams = embedConfig.shouldEncodeUrlQueryParams; | ||
|
|
||
| readyForRenderResolve(); | ||
| }); |
There was a problem hiding this comment.
This is far more complex than it needs to be.
this.isReadyForRenderPromise = getInitPromise().then(() => {
const embedConfig = getEmbedConfig();
this.embedConfig = embedConfig;
if (!embedConfig.authTriggerContainer && !embedConfig.useEventForSAMLPopup) {
this.embedConfig.authTriggerContainer = domSelector;
}
this.thoughtSpotHost = getThoughtSpotHost(embedConfig);
this.thoughtSpotV2Base = getV2BasePath(embedConfig);
this.shouldEncodeUrlQueryParams = embedConfig.shouldEncodeUrlQueryParams;
});
The isReadyForRender flag is not needed. We should just use the promise throughout
There was a problem hiding this comment.
yup forgot to remove isReadyForRender its not used.
yeah this impl is much concise and clean, missed it .
will update
src/embed/ts-embed.ts
Outdated
| } | ||
|
|
||
| return renderInQueue((nextInQueue) => { | ||
| return renderInQueue(async (nextInQueue) => { |
There was a problem hiding this comment.
Is the throwInitError method above needed to be changed ?
There was a problem hiding this comment.
yeah we can remove that , i dont think that was ever called
cause if init is not done , code always failed in getIframeSrc it never reached here.
getThoughtSpotHost throws erorr while creating new instance itself
is there any other scenario ?
src/embed/ts-embed.ts
Outdated
| uploadMixpanelEvent(MIXPANEL_EVENT.VISUAL_SDK_RENDER_START); | ||
| return getAuthPromise() | ||
|
|
||
| return this.isReadyForRenderPromise.then(() => getAuthPromise() |
There was a problem hiding this comment.
isn't authPromise enough here ?
There was a problem hiding this comment.
yeah true , updated
src/react/index.tsx
Outdated
| ref.current = authEE; | ||
| }, [config]); | ||
|
|
||
| return ref.current; |
There was a problem hiding this comment.
since you are returning the current this will always be undefined. You have to return the ref
There was a problem hiding this comment.
missed that , updated
|
SonarQube Quality Gate
|
No description provided.