add hmr for build #1008

MoonBall · 2020-09-07T10:41:21Z

Changes

Part1 of #1002.
I test it by run yarn build --watch --hmr command in create-snowpack-app/app-template-react directory.

Testing

no test added.

vercel · 2020-09-07T10:41:29Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/7434oity3
✅ Preview: Canceled

[update for d8042ca canceled]

FredKSchott

Nice! Glad to see that this looks pretty straightforward. Since we still don't have dev tests, can you confirm that you tested this manually yourself?

FredKSchott · 2020-09-08T05:13:30Z

snowpack/src/build/build-import-proxy.ts

@@ -75,7 +77,8 @@ export function wrapHtmlResponse({
  });

  if (hmr) {
-    const hmrScript = `<script type="module" src="${getMetaUrlPath('hmr.js', config)}"></script>`;
+    const setHmrUrlScript = hmrUrl ? `<script type="text/javascript">window.HMR_WEBSOCKET_URL="${hmrUrl}";</script>\n` : '';


no need for this, we already have a section in our docs telling users how to configure this themselves outside of Snowpack.

Our default port is 12321. how to inform users what is url of websocket if we don't set HMR_WEBSOCKET_URL by ourselves?

There's definitely still work to be done here, but I'd rather do it all together in a follow up PR and leave this existing limitation in place for now.

The reason is that this was always a bandaid, and now we may just want to move this entirely into a Snowpack config value, and then remove the window. HMR_WEBSOCKET_URL snippet entirely.

Because ESM-HMR is shared we can't remove window.HMR_WEBSOCKET_URL entirely, but Snowpack could inject it into the hmr.js file itself, instead:

// hmr.js // injected by Snowpack during dev & build, if config value is set window.HMR_WEBSOCKET_URL = ${"VALUE FROM SNOWPACK CONFIG} // the static hmr.js file contents ...

snowpack/src/commands/build.ts

FredKSchott · 2020-09-08T05:16:37Z

snowpack/src/commands/build.ts

@@ -449,6 +460,10 @@ export async function command(commandOptions: CommandOptions) {
        await changedPipelineFile.writeProxyToDisk(builtFile);
      }
    }
+
+    if (hmrEngine) {
+      hmrEngine.broadcastMessage({type: 'reload'});


snowpack/src/hmr-server-engine.ts

snowpack/src/commands/build.ts

MoonBall · 2020-09-08T06:25:33Z

Nice! Glad to see that this looks pretty straightforward. Since we still don't have dev tests, can you confirm that you tested this manually yourself?

I tested and I will test again after fix these nits.

FredKSchott

Looking good! A few more comments added, but overall direction is 👍

snowpack/src/util.ts

FredKSchott · 2020-09-10T23:23:05Z

snowpack/src/build/build-import-proxy.ts

@@ -75,7 +77,8 @@ export function wrapHtmlResponse({
  });

  if (hmr) {
-    const hmrScript = `<script type="module" src="${getMetaUrlPath('hmr.js', config)}"></script>`;
+    const setHmrUrlScript = hmrUrl ? `<script type="text/javascript">window.HMR_WEBSOCKET_URL="${hmrUrl}";</script>\n` : '';


There's definitely still work to be done here, but I'd rather do it all together in a follow up PR and leave this existing limitation in place for now.

The reason is that this was always a bandaid, and now we may just want to move this entirely into a Snowpack config value, and then remove the window. HMR_WEBSOCKET_URL snippet entirely.

Because ESM-HMR is shared we can't remove window.HMR_WEBSOCKET_URL entirely, but Snowpack could inject it into the hmr.js file itself, instead:

// hmr.js // injected by Snowpack during dev & build, if config value is set window.HMR_WEBSOCKET_URL = ${"VALUE FROM SNOWPACK CONFIG} // the static hmr.js file contents ...

FredKSchott · 2020-09-10T23:28:35Z

snowpack/src/commands/build.ts


 const CONCURRENT_WORKERS = require('os').cpus().length;

+let hmrEngine: EsmHmrEngine | null = null;
+function getIsHmrEnabled(config: SnowpackConfig) {
+  return config.buildOptions.watch && config.devOptions.hmr;


So I just checked config.ts, and config.buildOptions.watch is true by default. This is a bit tricky since we originally talked about HMR being opt-in for build (at least to start, until we are more confident that it wouldn't be a breaking change).

I'm kind of torn, should we just ship this as opt-in for build? I just think it's so unexpected to be running a separate server during the build command. In that case, we need to remove the default hmr value from DEFAULT_CONFIG and then handle it as default "on" for dev so that this code you have here is opt-in and runs as expected.

// in dev.ts - const {hmr: isHmr} = config.devOptions; + const isHmr = config.devOptions.hmr || true;

There's definitely still work to be done here, but I'd rather do it all together in a follow up PR and leave this existing limitation in place for now.

maybe we should print a message to inform user about web socket's address.

Edit: I deleted the script of setting url of web socket.

Edit: add message in https://github.com/pikapkg/snowpack/pull/1008/files#diff-9e8fbf411449d99e70310e93d8119617R302

Yup, that makes sense to me

In that case, we need to remove the default hmr value from DEFAULT_CONFIG and then handle it as default "on" for dev so that this code you have here is opt-in and runs as expected.

The config.devOptions.hmr will be undefined and not match our config schema if we don't set the hmr value in loadAndValidateConfig() function. I prefer to set hmr value in loadAndValidateConfig() function according to dev or build.

Our config functions don't know what command they are being run in.

We give commands the chance to modify the config at startup, but at that point it's too late to tell whether the original option was undefined or true/false.

It's okay to make this optional in the config schema as well, there are several places where we already do this. The current behavior isn't changing, just the hmr config value would now support undefined, which lets the different commands handle this case differently.

Ok, modified 😁

MoonBall · 2020-09-11T06:59:13Z

I tested 4 cases.

yarn start --reload. There is hmr code in html.
yarn start --reload --no-hmr. There is no hmr code in html.
yarn build --clean --watch. There is no hmr's message shown on console.
yarn build --clean --watch --hmr and add <script type="text/javascript">window.HMR_WEBSOCKET_URL="ws://localhost:12321";</script> in HTML file. There is hmr's message shown on console and hmr does work in browser.

FredKSchott

Awesome stuff, LGTM!

jeberly · 2020-09-12T20:56:05Z

Great work guys! I just pulled it down, and tested in our app and it works great! Very simple. snowpack build --clean --watch --hmr --no-minify --no-bundle and include the window.HMR_WEBSOCKET_URL="ws://localhost:12321" in our dev js.

Loving snowpack.

francislavoie · 2020-12-01T09:33:48Z

I'll just say I still haven't gotten build --clean --watch --hmr working, I wrote about it here but didn't get an answer yet: #1459 🤔

joshnuss · 2020-12-11T06:45:15Z

snowpack build --watch --hmr didn't work for me either.

When I add <script type="module">console.log(import.meta.hot)</script> and run snowpack build --watch --hmr it prints undefined. When I run snowpack dev it work fine and returns a HotModuleState{}.

I believe it's because wrapImportMeta() is called with hmr: false from the build command. It results in createHotContext() not being called

FredKSchott · 2020-12-11T23:46:00Z

I think there are a few convos going on related to this issue. I just tried to reproduce and can confirm that snowpack build --watch. --hmr is working BUT it looks like it only supports basic browser-sync functionality. On change, the browser is automatically refreshed but import.meta.accept() is never called and full HMR never kicks in. You can see why here: #1008 (comment)

The HMR server needs to understand your application to properly bundle HMR events to the correct accept() handler. In dev, we scan files as we serve them. In build, we should scan them when we build them.

Issue added to prioritize here: #1935

MoonBall requested a review from a team as a code owner September 7, 2020 10:41

vercel bot temporarily deployed to Preview September 7, 2020 10:41 Inactive

MoonBall force-pushed the feat/hmr-for-build branch from 7520e9a to 5da274f Compare September 7, 2020 10:41

vercel bot temporarily deployed to Preview September 7, 2020 10:41 Inactive

FredKSchott reviewed Sep 8, 2020

View reviewed changes

MoonBall force-pushed the feat/hmr-for-build branch from 5da274f to 5666c15 Compare September 8, 2020 06:38

vercel bot temporarily deployed to Preview September 8, 2020 06:38 Inactive

FredKSchott reviewed Sep 10, 2020

View reviewed changes

MoonBall force-pushed the feat/hmr-for-build branch from 5666c15 to dcd347c Compare September 11, 2020 04:04

vercel bot temporarily deployed to Preview September 11, 2020 04:04 Inactive

add hmr for build

d8042ca

MoonBall force-pushed the feat/hmr-for-build branch from dcd347c to d8042ca Compare September 11, 2020 06:52

vercel bot temporarily deployed to Preview September 11, 2020 06:52 Inactive

FredKSchott approved these changes Sep 11, 2020

View reviewed changes

FredKSchott merged commit e4faa08 into FredKSchott:master Sep 11, 2020

This was referenced Sep 11, 2020

Configure HMR Port/URL #1041

Closed

Build --watch HMR Support #1002

Closed

FredKSchott mentioned this pull request Dec 11, 2020

Implement full HMR in snowpack build --watch mode #1935

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add hmr for build #1008

add hmr for build #1008

MoonBall commented Sep 7, 2020

vercel bot commented Sep 7, 2020 •

edited

Loading

FredKSchott left a comment

FredKSchott Sep 8, 2020

MoonBall Sep 8, 2020

FredKSchott Sep 10, 2020

FredKSchott Sep 8, 2020

MoonBall commented Sep 8, 2020

FredKSchott left a comment

FredKSchott Sep 10, 2020

FredKSchott Sep 10, 2020 •

edited

Loading

MoonBall Sep 11, 2020 •

edited

Loading

FredKSchott Sep 11, 2020

MoonBall Sep 11, 2020

FredKSchott Sep 11, 2020

MoonBall Sep 11, 2020

MoonBall commented Sep 11, 2020

FredKSchott left a comment

jeberly commented Sep 12, 2020

francislavoie commented Dec 1, 2020

joshnuss commented Dec 11, 2020

FredKSchott commented Dec 11, 2020

add hmr for build #1008

add hmr for build #1008

Conversation

MoonBall commented Sep 7, 2020

Changes

Testing

vercel bot commented Sep 7, 2020 • edited Loading

FredKSchott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoonBall commented Sep 8, 2020

FredKSchott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FredKSchott Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

MoonBall Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoonBall commented Sep 11, 2020

FredKSchott left a comment

Choose a reason for hiding this comment

jeberly commented Sep 12, 2020

francislavoie commented Dec 1, 2020

joshnuss commented Dec 11, 2020

FredKSchott commented Dec 11, 2020

vercel bot commented Sep 7, 2020 •

edited

Loading

FredKSchott Sep 10, 2020 •

edited

Loading

MoonBall Sep 11, 2020 •

edited

Loading