Script and useScript #2194
Script and useScript #2194
Conversation
|
We detected some changes in |
| import {ScriptState, UseScriptProps} from './types.js'; | ||
|
|
||
| export function useScript(props: UseScriptProps) { | ||
| const {pathname} = useUrl(); |
There was a problem hiding this comment.
@frehner — This is the only hydrogren dependency that would prevent this from being part of h-ui. We could maybe replace it with a framework agnostic hook. Any thoughts?
There was a problem hiding this comment.
To be honest, I would have to see what it would look like in other frameworks and if it would actually be valuable there or not. For example, if I'm using NextJS, why would I use this over their official component?
There was a problem hiding this comment.
That's a valid point but some frameworks like Remix and Gatsby don't offer a Script alternative.
There was a problem hiding this comment.
I would see a lot of value in ditching this dependency. Do we need to reach for a hook here to get the pathname.
| @@ -0,0 +1,24 @@ | |||
| // polyfill for Safari and IE | |||
| export const requestIdleCallback = | |||
There was a problem hiding this comment.
This is nextjs's polyfill. @frehner this is another dependency to enable load="onIndle". Thoughts for a h-ui context?
| @@ -0,0 +1,52 @@ | |||
| // simulate a CDN/server delivering 3rd-party scripts | |||
| export async function api(request) { | |||
There was a problem hiding this comment.
This is only needed for e2e tests
| | Name | Type | Description | | ||
| | -------------- | ------------------------------------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | `target` | <code>"head" \| "body" (default)</code> | The target DOM element where the script should be inserted. This feature is only available to non-inline loading strategies such as "afterHydration", "inWorker" and "onIdle" | | ||
| | `id` | <code>string (required)</code> | A unique identifier for the script. The id will be used as the key of the script. | |
There was a problem hiding this comment.
If id is required, shouldn't it be in the above example?
|
|
||
| ### `beforeHydration` | ||
|
|
||
| These scripts are inlined and hence considered render-blocking. This strategy is mainly recommended for scripts aiming to set global `window` properties, configurations or event listeners. These scripts can only be included in `App.server.tsx`. This ensures that they are run on any initial route. |
There was a problem hiding this comment.
| These scripts are inlined and hence considered render-blocking. This strategy is mainly recommended for scripts aiming to set global `window` properties, configurations or event listeners. These scripts can only be included in `App.server.tsx`. This ensures that they are run on any initial route. | |
| Scripts using the `beforeHydration` strategy are inlined and render-blocking. This strategy is only recommended for scripts that set global `window` properties, configurations or event listeners. These scripts can only be included in `App.server.tsx`, which ensures they are run on any initial route. |
There was a problem hiding this comment.
#The that after ensures is correct, let's put it back in!
To @cartogram's edit, would add an Oxford comma between configurations and event listeners
Verify throughout that you're using the Oxford comma
|
|
||
| ### `afterHydration` | ||
|
|
||
| These scripts are loaded, injected and run after react has finished hydrating on the client (via a useEffect). In addition, these scripts include additional props to further customize their behavior `target`, `reload`, `onLoad`, `onReady` and `onError` callbacks. |
There was a problem hiding this comment.
| These scripts are loaded, injected and run after react has finished hydrating on the client (via a useEffect). In addition, these scripts include additional props to further customize their behavior `target`, `reload`, `onLoad`, `onReady` and `onError` callbacks. | |
| Scripts using the `afterHydration` strategy are loaded, injected and run after React has finished hydrating on the client (via a `useEffect`). In addition, these scripts accept props to further customize the behavior of the `target`, `reload`, `onLoad`, `onReady` and `onError` callbacks. |
There was a problem hiding this comment.
The beforeHydration strategy gives an example how or why it would be used. Maybe provide the same for afterHydration.
| <Script | ||
| id="oi-children-reload" | ||
| load="afterHydration" | ||
| reload={true} |
There was a problem hiding this comment.
| reload={true} | |
| reload |
Make this change for all of these I think.
| id="oi-src-reload" | ||
| src="//example.com/script.js" | ||
| load="afterHydration" | ||
| reload={true} |
There was a problem hiding this comment.
| reload={true} | |
| reload |
| src="//example.com/script.js" | ||
| /> | ||
|
|
||
| // with a src and with reload |
There was a problem hiding this comment.
| // with a src and with reload | |
| // with a src and reload |
|
|
||
| // with a src | ||
| <Script | ||
| id="oi-src" |
There was a problem hiding this comment.
| id="oi-src" | |
| id="ah-src" |
|
|
||
| // with a src and with reload | ||
| <Script | ||
| id="oi-src-reload" |
There was a problem hiding this comment.
| id="oi-src-reload" | |
| id="ah-src-reload" |
|
|
||
| // with callbacks | ||
| <Script | ||
| id="oi-src-cbs" |
There was a problem hiding this comment.
| id="oi-src-cbs" | |
| id="ah-src-cbs" |
|
|
||
| ### `onIdle` | ||
|
|
||
| These scripts are loaded, injected and run when the main thread is idle (via requestIdleCallback). In addition, these scripts include additional props to further customize their behavior `target`, `reload`, `onLoad`, `onReady` and `onError` callbacks. |
There was a problem hiding this comment.
If you end up making the suggestions above, please apply them to this paragraph as well.
| <> | ||
| // Loading google analytics.js via a web worker | ||
| <Script | ||
| id="inWorker-analytics" |
There was a problem hiding this comment.
| id="inWorker-analytics" | |
| id="iw-analytics" |
I don't really care how these IDs are named as long as it is consistent.
|
Just read through the docs, super exciting!!! It seems like there are a lot of different loading strategies for different use cases but I don't have a clear sense which one I should use when. In other words, are there any heuristics I should consider when setting the loading strategy for a given script? |
| | `src` | <code>string</code> | A URL string. This string can be an absolute path or a relative path depending on the location of the third-party script. The `src` prop is required if `dangerouslySetInnerHTML` or `children` are not used | | ||
| | `dangerouslySetInnerHTML` | <code>string</code> | Any valid javascript code | | ||
| | `children` | <code>string \| string[]</code> | Any valid javascript code | | ||
| | `load` | <code>"beforeHydration" \| "afterHydration" (default) \| "inWorker" \| "onIdle"</code>| The loading strategy. See Loading strategies for more info. | |
There was a problem hiding this comment.
I think we might want to bikeshed these strategy names
| response = new Response(body, { | ||
| headers: { | ||
| 'content-type': contentType, | ||
| 'Access-Control-Allow-Origin': url.origin, |
There was a problem hiding this comment.
Is an Access-Control-Allow_Origin header necessary? Just by going through the proxy we are already on the same domain right?
There was a problem hiding this comment.
Looks good, @juanpprieto! Just some style/organization/copy nits. After you address the comments, I'll take another pass and 🎩 on .dev
| --- | ||
| gid: 5848138c-3dbb-11ed-b878-0242ac120002 | ||
| title: Script | ||
| description: The Script component renders a script tag |
There was a problem hiding this comment.
| description: The Script component renders a script tag | |
| description: The Script component renders a script tag. |
| <aside class="note beta"> | ||
| <h4>Experimental feature</h4> | ||
|
|
||
| <p>Hydrogen Script is an experimental feature. As a result, functionality is subject to change. You can provide feedback on this feature by <a href="https://github.com/Shopify/hydrogen/issues">submitting an issue in GitHub</a>.</p> |
There was a problem hiding this comment.
| <p>Hydrogen Script is an experimental feature. As a result, functionality is subject to change. You can provide feedback on this feature by <a href="https://github.com/Shopify/hydrogen/issues">submitting an issue in GitHub</a>.</p> | |
| <p>The `Script` component is an experimental feature. As a result, functionality is subject to change. You can provide feedback on this feature by <a href="https://github.com/Shopify/hydrogen/issues">submitting an issue in GitHub</a>.</p> |
|
|
||
| </aside> | ||
|
|
||
| The `Script` component is an enhanced HTML [`<script>`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script) element. Script enables efficient loading third-party scripts by offering different loading strategies within the application lifecycle. |
There was a problem hiding this comment.
| The `Script` component is an enhanced HTML [`<script>`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script) element. Script enables efficient loading third-party scripts by offering different loading strategies within the application lifecycle. | |
| The `Script` component is an enhanced HTML [`<script>`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script) element. It offers different loading strategies within the application lifecycle. Use `Script` to load third-party scripts efficiently. |
Maybe a use case here, like a for example, to help orient partners on when to use.
|
|
||
| ## Example code | ||
|
|
||
| Load google analytics only after react has finished hydrating on the client. "afterHydration" is the default loading strategy |
There was a problem hiding this comment.
| Load google analytics only after react has finished hydrating on the client. "afterHydration" is the default loading strategy | |
| Load Google Analytics only after React has finished hydrating on the client. 'afterHydration' is the default loading strategy. |
Throughout, review for correct capitalization on Google
|
|
||
| Load google analytics only after react has finished hydrating on the client. "afterHydration" is the default loading strategy | ||
|
|
||
| {% codeblock file, filename: 'App.server.jsx' %} |
There was a problem hiding this comment.
| {% codeblock file, filename: 'App.server.jsx' %} | |
| {% codeblock file, filename: 'App.server.jsx' %} | |
hypo-nit. Just a linting thing on the shopify-dev repo. Would apply throughout
| | `id` | Yes | A unique identifier for the script tag | | ||
| | `url` | Yes | The URL string for the external script. | | ||
| | `target` | <code>"head" \| "body" (default)</code> | The target DOM element where the script should be inserted. This feature is only available to non-inline loading strategies such as | ||
| | `load` | <code>"afterHydration" (default) \| "onIdle"</code>| The loading strategy. See loading strategies [`section`](https://shopify.dev/api/hydrogen/components/primitive/script#loading-strategies) for more info. | |
There was a problem hiding this comment.
| | `load` | <code>"afterHydration" (default) \| "onIdle"</code>| The loading strategy. See loading strategies [`section`](https://shopify.dev/api/hydrogen/components/primitive/script#loading-strategies) for more info. | | |
| | `load` | <code>"afterHydration" (default) \| "onIdle"</code>| The loading strategy. For more information, refer to [loading strategies](https://shopify.dev/api/hydrogen/components/primitive/script#loading-strategies).| |
Not sure, but looks like you'd edit this so that the link is over loading strategies.
| | `url` | Yes | The URL string for the external script. | | ||
| | `target` | <code>"head" \| "body" (default)</code> | The target DOM element where the script should be inserted. This feature is only available to non-inline loading strategies such as | ||
| | `load` | <code>"afterHydration" (default) \| "onIdle"</code>| The loading strategy. See loading strategies [`section`](https://shopify.dev/api/hydrogen/components/primitive/script#loading-strategies) for more info. | | ||
| | `reload` | <code>boolean (default false)</code> | Scripts rendered with this option will be reloaded after every page navigation (if available on the next route). This option is only available in "afterHydration" and "onIdle" loading strategies. | |
There was a problem hiding this comment.
| | `reload` | <code>boolean (default false)</code> | Scripts rendered with this option will be reloaded after every page navigation (if available on the next route). This option is only available in "afterHydration" and "onIdle" loading strategies. | | |
| | `reload` | <code>boolean (default `false`)</code> | Scripts rendered with this option are reloaded after every page navigation (if available on the next route). This option is only available in "afterHydration" and "onIdle" loading strategies. | |
can apply the previous edits here. Code formatting, links, tightening up the phrase (if acceptable).
|
|
||
| ## Return value | ||
|
|
||
| The `useLoadScript` hook returns the following values that allow you to understand the state of the external script you are loading: |
There was a problem hiding this comment.
| The `useLoadScript` hook returns the following values that allow you to understand the state of the external script you are loading: | |
| The `useLoadScript` hook returns the following values for understanding the state of the external script that you're loading: |
verify contraction use throughout
| | `load` | <code>"afterHydration" (default) \| "onIdle"</code>| The loading strategy. See loading strategies [`section`](https://shopify.dev/api/hydrogen/components/primitive/script#loading-strategies) for more info. | | ||
| | `reload` | <code>boolean (default false)</code> | Scripts rendered with this option will be reloaded after every page navigation (if available on the next route). This option is only available in "afterHydration" and "onIdle" loading strategies. | | ||
|
|
||
| ## Return value |
There was a problem hiding this comment.
| ## Return value | |
| ## Return values |
|
|
||
| | Value | Description | | ||
| | --------- | ----------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | `loading` | The script is still loading. For example, the script tag can be on the page but the resource might not be fully loaded yet while in this state. | |
There was a problem hiding this comment.
| | `loading` | The script is still loading. For example, the script tag can be on the page but the resource might not be fully loaded yet while in this state. | | |
| | `loading` | The script is still loading. For example, the script tag can be on the page but the resource might not be fully loaded yet. | |
| ], | ||
| } | ||
| : {}; | ||
| // console.log({suggestions}); |
| @@ -325,7 +325,7 @@ If your Store is based on the "Demo Store" tempate, and you are using the `test: | |||
| } from '@shopify/hydrogen/platforms'; | |||
|
|
|||
| // Platform entry handler | |||
| export default function(request) { | |||
| export default function (request) { | |||
| } | ||
|
|
||
| /* | ||
| If we are not loading and have and exiting idle callback and |
There was a problem hiding this comment.
| If we are not loading and have and exiting idle callback and | |
| If we are not loading and exiting idle callback and |
| const hasLoaded = key && LoadCache.has(key); | ||
|
|
||
| // track last pathname | ||
| PrevPathCache.set(key, pathname); |
There was a problem hiding this comment.
Only bother doing this if it pathnameChanged.
| reload = false, | ||
| } = props; | ||
|
|
||
| const key = (id ?? '') + (src ?? ''); |
There was a problem hiding this comment.
| const key = (id ?? '') + (src ?? ''); | |
| const key = `${id}${key}` |
Should be fine given your types.
| */ | ||
| export function loadScriptOnIdle( | ||
| props: ScriptProps, | ||
| cb: (status: boolean) => void = () => {} |
There was a problem hiding this comment.
What do you think about passing the entire ScriptResponse to the call back?
| : ''; | ||
| } | ||
|
|
||
| // Spread <Script /> props as <scrip /> attributes |
There was a problem hiding this comment.
| // Spread <Script /> props as <scrip /> attributes | |
| // Pass <Script /> component props as <script /> tag attributes if they are valid HTML attributes |
| } | ||
|
|
||
| /* | ||
| marks this script to be loaded by partytown inWorker |
There was a problem hiding this comment.
| marks this script to be loaded by partytown inWorker | |
| marks this script to be loaded by PartyTown inWorker |
| continue; | ||
| } | ||
|
|
||
| // map react props to DOM attribute names |
There was a problem hiding this comment.
| // map react props to DOM attribute names | |
| // map React props to DOM attribute names |
| } else { | ||
| return Promise.resolve({ | ||
| status: false, | ||
| event: 'Script load promise not found', |
There was a problem hiding this comment.
I think we can be more consistent and helpful with these events. I'm not yet sure how they are meant to be used.
There was a problem hiding this comment.
A few more comments on files I didn't get around to reviewing on the first pass, but overall this is looking really great!
In the future, I would have loved to see this broken up in PRs that are a fraction of the size. Lint rules and such can always come in subsequent PRs to make things more palatable for reviewers and overall less risky 🙂 I know this was a beast of a feature and appreciate all the work you put in to get this to this point! ❤️
| docs: { | ||
| description: `Prevent using ${new (Intl as any).ListFormat('en').format( | ||
| SCRIPT_CALLBACKS | ||
| )} <Script /> callback(s) in server components`, |
There was a problem hiding this comment.
| )} <Script /> callback(s) in server components`, | |
| )} <Script /> callbacks in server components`, |
|
|
||
| context.report({ | ||
| node, | ||
| data: {callback: callbackAttributes.join(', ')}, |
There was a problem hiding this comment.
instead of join, what about using the same ListFormat niceness from above. IE:
new (Intl as any).ListFormat('en').format(
callbackAttributes
)
| meta: { | ||
| type: 'problem', | ||
| docs: { | ||
| description: `Prevent using ${new (Intl as any).ListFormat('en').format( |
There was a problem hiding this comment.
do we still need the any? We may have updated the typings since I added the other rules.
|
|
||
| function ServerComponent() { | ||
| return ( | ||
| <Script onReady={() => {}} onLoad={() => {}} onError /> |
There was a problem hiding this comment.
Can we also test multiple Script components in the same Component.
| return <Script onError={() => {}} />; | ||
| } | ||
| `, | ||
| filename: 'ServerComponent.client.tsx', |
There was a problem hiding this comment.
Adds tests for multiple callbacks and multiple components.
| @@ -0,0 +1,31 @@ | |||
| # Prefer using the `Script` component instead of HTML `script` tags | |||
|
|
|||
| Scripts loaded at the wrong time can negatively [Core Web Vital](https://web.dev/vitals/) that [Google uses in search ranking](https://developers.google.com/search/blog/2020/05/evaluating-page-experience) performance. | |||
There was a problem hiding this comment.
I think we can use this opportunity to educate the user more on what the "wrong time" means, ie: not blocking the main thread, workers, partytown, etc... I know this information is written extensively elsewhere in our docs so not expecting an MDN article on the subject, but just a few sentences to drive home the value of why this rule exists (with links) would be awesome. Also, I might even add like a "Additional resources" heading at the bottom. Thoughts?
cc @rennyG
There was a problem hiding this comment.
yep I think that's a great idea. If this is written extensively elsewhere, could consider linking out to those pages for devs that want to learn more
| @@ -0,0 +1,3 @@ | |||
| ## Rule details | |||
| @@ -0,0 +1,14 @@ | |||
| // Examples of **correct** code for this rule: | |||
There was a problem hiding this comment.
delete this directory too. Lets move everything to a single Readme for each rule.
| import {createRule} from '../../utilities'; | ||
|
|
||
| const SCRIPT_TAG_NAME = 'Script'; | ||
| // TODO: adapt to other frameworks (e.g. Next.js requires beforeInteractive @ `pages/_document.js`) |
| }); | ||
|
|
||
| function error(options: {suggestion?: string; messageId?: string} = {}) { | ||
| console.log(options); |
| <script | ||
| key={id + js.slice(0, 24)} | ||
| {...props} | ||
| suppressHydrationWarning |
There was a problem hiding this comment.
Curious, why is it a mismatch in this case?
| if (load === 'inWorker') { | ||
| script.setAttribute('type', 'text/partytown'); | ||
| } |
There was a problem hiding this comment.
Can we somehow notify if this is set but Partytown is not installed? Like, is there anything in window. we can check of similar?
Description
This PR introduces the following
<Script />— A script component supporting multiple loading strategies and reload.useScript— A hook versionAdditional context
useLoadScripthas performance limitations due to its eager nature.Notes
-- prefer-script-component
-- scripts-in-layout-components
playground/async-config/tests/e2e-test-cases.tsBefore submitting the PR, please make sure you do the following:
fixes #123)yarn changeset addif this PR cause a version bump based on Keep a Changelog and adheres to Semantic Versioning