[Themes] PoC Hot Reload for OSE #4908
Conversation
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success1946 tests passing in 885 suites. Report generated by 🧪jest coverage report action from 38bd7c1 |
…ors in OSE when CLI is not active
|
/snapit |
|
🫰✨ Thanks @frandiox! Your snapshot has been published to npm. Test the snapshot by intalling your package globally: pnpm i -g @shopify/cli@0.0.0-snapshot-20250110184614
|
| startDataReload: async (signal: AbortSignal) => { | ||
| if (!isOSE) return null | ||
|
|
||
| // Force OSE to show the loading state | ||
| window.dispatchEvent(new Event('pagehide')) | ||
|
|
||
| return fetch(window.location.href, { | ||
| // Note: enable these properties when we have access to replace_templates | ||
| // method: 'POST', | ||
| // body: storefrontReplaceTemplatesParams(data.replaceTemplates), | ||
| // This is required to get the OnlineStoreEditorData script: | ||
| headers: {Accept: 'text/html'}, | ||
| signal, | ||
| }) | ||
| .then((response) => response.text()) | ||
| .catch((error) => { | ||
| logError('Error fetching full page reload for section settings', error) | ||
| return null | ||
| }) | ||
| }, | ||
| finishDataReload: async (oseDataPromise: Promise<string | null>) => { | ||
| if (!isOSE) return null | ||
|
|
||
| const refreshedHtml = await oseDataPromise | ||
| const newOSEData = new DOMParser() | ||
| .parseFromString(refreshedHtml ?? '', 'text/html') | ||
| .querySelector('#OnlineStoreEditorData')?.textContent | ||
|
|
||
| if (newOSEData) { | ||
| const oseDataElement = document.querySelector('#OnlineStoreEditorData') | ||
| if (oseDataElement && newOSEData !== oseDataElement.textContent) { | ||
| oseDataElement.textContent = newOSEData | ||
| logInfo('OSE data updated') | ||
| } | ||
| } | ||
|
|
||
| // OSE reads the new data after the page is loaded | ||
| window.dispatchEvent(new Event('load')) | ||
| }, |
There was a problem hiding this comment.
We could remove these actions since they are not used yet. Using them is faster but it seems to create some small issues with OSE state.
There was a problem hiding this comment.
Thank you for this PR, @frandiox 🔥 Amazing work as usual!
I couldn't spot any issues with themes neither theme app extension. Given the nature of this change tho, I'd love to expand these tests even further.
When we feel ready, I believe we can snapit this branch and share a build with our partners to gather their feedback before the release. I'm sure they will love having hot reload on OSE, so we will likely have a wide audience.
Regarding these changes:
- Move the hot reload decisions from the server to the client. For example, instead of sending an event for "CSS update" from the server to the client, now the server sill simply say "here, this file has been changed". The client will check the extension of the file and decide to perform a CSS update, or something else.
This is very neat. The more I look at the client.ts file, the more it feels like it should be a standalone library. I can see it being a dependency for the CLI and for the OSE/theme-preview, and I can also see third-party tooling being built on top of it.
Having that as a standalone library might also give us more room to separate the logic and version those APIs—keeping in mind that OSE won't have guarantees about the CLI version.
- The server now emits 2 events per file: once when the file is updated locally, and another one when it's uploaded to the cloud
This is an excellent decision! ✨
Please, let me know your thoughts about this! Thanks again for this PR!
| // - Local preview in the CLI: the URL is like localhost:<port> | ||
| // - OSE visual preview: the URL is a myshopify.com domain | ||
| // - Theme Preview: the URL is a myshopify.com domain |
There was a problem hiding this comment.
Thinking about these consumers, I believe we could consider extracting this from the CLI and turning client.ts into a standalone package.
For Shopify CLI, it would be a dependency, but having it as a standalone package (and possibly a standalone repository) might help us with bundling and making this asset available for OSE/theme-preview.
There was a problem hiding this comment.
The thing is, if we inject this in SFR for theme-preview... we would get it automatically injected also in the CLI.
We could always overwrite it at the proxy stage with custom stuff but since we control every part, maybe we can just rely on it? The code connects to the localhost:<port> if available automatically, so it would know it runs under the CLI.
In this case, it almost makes more sense for this client.ts to live in SFR directly? :confused-parrot:
It could still be good to have it as a library to import types I guess? So maybe in theme-tools as theme-hot-reload?
| } | ||
|
|
||
| export interface HotReloadEventPayload { | ||
| isAppExtension?: boolean |
There was a problem hiding this comment.
Generally, we refer to TAEs as theme extensions in the code base, I believe we could rename this to:
| isAppExtension?: boolean | |
| isThemeExtension?: boolean |
wip
Goals: support hot reload for Online Store Editor and Theme Preview.
The changes in this repo are: