NostoProvider: Extract client script loading functionality to a hook #101
Conversation
src/hooks/useClientScriptLoad.ts
Outdated
|
|
||
| if ( | ||
| !existingScript || | ||
| existingScript?.getAttribute("nosto-language") !== language || |
There was a problem hiding this comment.
Is the language check needed?
There was a problem hiding this comment.
May need it for markets. I can't say for sure though
There was a problem hiding this comment.
this is a language mismatch check, it makes sense
nosto-mikpou
force-pushed
the
extract-client-script-to-hook
branch
from
d73ff7e to
ee169bb
Compare
src/hooks/useClientScriptLoad.ts
Outdated
| // Load Nosto client script if not already loaded externally | ||
| if (!isNostoLoaded() && !shopifyMarkets) { | ||
| const urlPartial = `/include/${account}` | ||
| const script = createScriptElement(urlPartial) |
There was a problem hiding this comment.
Here, can we have a check if the client script is already loaded on page and skip this if possible? Or else we could introduce a config flag through which we could skip adding duplicate script tags
There was a problem hiding this comment.
In a separate PR also fine
There was a problem hiding this comment.
Sure, we have loadScript flag in the WIP PR waiting to be transferred in the next PR. Sorry, I jumped to different topic. That flag was for the externally loaded script. Timo's comment below is the correct one.
There was a problem hiding this comment.
there is already a loaded check (isNostoLoaded) and the flag will be introduced in a later PR
src/hooks/useClientScriptLoad.ts
Outdated
|
|
||
| type NostoScriptProps = Pick<NostoProviderProps, "account" | "host" | "shopifyMarkets"> | ||
|
|
||
| export function useClientScriptLoad(props: NostoScriptProps) { |
There was a problem hiding this comment.
rename this to useLoadClientScript or useClientScriptLoader
There was a problem hiding this comment.
useLoadClientScript would be my preference
Extracts client script functionality to a useClientScriptLoad hook + minor cleanup