tri review #68
tri review #68
Conversation
| } | ||
|
|
||
| // will be usefull when all data are retrieved async | ||
| async _initialize(){ |
There was a problem hiding this comment.
Can you explain why this will be useful?
What is difference between the constructor and the initialise function?
There was a problem hiding this comment.
In the future, all data may be retrieved asynchronously. Just setting things up.
| } | ||
|
|
||
| showConsentTool() { | ||
| async showConsentTool() { |
There was a problem hiding this comment.
Can you please explain the changes here?
There was a problem hiding this comment.
the async is not really important, i just forgot to delete this change.
The real changes are the ones below this line.
|
|
||
| export default function initCmp(loaderData) { | ||
| export default async function initCmp(loaderData) { | ||
|
|
There was a problem hiding this comment.
Again please can you explain the benefits of the suggested approach?
There was a problem hiding this comment.
Better readability. Assuming we know how async/await works
| const isCookiePresent = (typeof cookie === 'string'); | ||
| console.log('[INFO][Module-isShowUi]: ', !isCookiePresent); | ||
| return Promise.resolve(!isCookiePresent); | ||
| // make it sync |
There was a problem hiding this comment.
If we make it sync, it cannot be used in a promise chain right?
There was a problem hiding this comment.
yes.. but by nature, it is not async. so we dont have to make it return promise.
edit: actually, we can use it in promise chain. but not on the start of chain.
|
|
||
| // prevent this script from running without window object | ||
| // e.g: SSR | ||
| if(typeof window === 'undefined') return; |
There was a problem hiding this comment.
But we need to try init() again?
There was a problem hiding this comment.
yes, in the browser.
For certain library (anything with node.js + SSR), Javascript are run in the server. So if this is run in the server, there will be no ERROR saying: cannot find {property} of undefined, or something like that
| .catch(err => console.error(err)); | ||
|
|
||
| await initApi(cmp); | ||
| const result = isShowUi(loaderData.iabCookie) ? await cmp.showConsentTool() : true; |
There was a problem hiding this comment.
This feels less readable, can you explain the benefit?
| const result = isShowUi(loaderData.iabCookie) ? await cmp.showConsentTool() : true; | ||
|
|
||
| // why do we need window.__cpm ? | ||
| cmp.readyCmpAPI(result); |
There was a problem hiding this comment.
I agree window is not good to attach to, but it is to stop having to import the cmp object in all files, maybe it should be created and exported elsewhere?
My reviews are mostly about promise, and the use of async.
Other than that:
windowdoesn't existswindow.cmptagManagerModule