automatically configure wsPort in html file #1147
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/ixf69davx |
| @@ -75,7 +75,12 @@ export function wrapHtmlResponse({ | |||
| }); | |||
|
|
|||
| if (hmr) { | |||
| const hmrScript = `<script type="module" src="${getMetaUrlPath('hmr-client.js', config)}"></script><script type="module" src="${getMetaUrlPath('hmr-error-overlay.js', config)}"></script>`; | |||
| let hmrScript = ``; | |||
| if (mode === 'production' && config.devOptions.hmrPort) { | |||
There was a problem hiding this comment.
It's not a good way to check whether mode is production. need your suggestions.
There was a problem hiding this comment.
Why only production? Does this not work in dev mode?
There was a problem hiding this comment.
bump! would love an answer so that we can resolve and merge!
There was a problem hiding this comment.
@FredKSchott sorry, I've been very busy recently.
Why only production? Does this not work in dev mode?
It's a good problem. I think hmrPort should work in dev mode 🤣.
There was a problem hiding this comment.
@FredKSchott I made a fix and the hmrPort option would take effect in dev mode. I manually test 6 cases above.
MoonBall
force-pushed
the
feat/configure-hmr-port
branch
from
c2f903b to
829c621
Compare
MoonBall
force-pushed
the
feat/configure-hmr-port
branch
from
829c621 to
0c29ce3
Compare
MoonBall
force-pushed
the
feat/configure-hmr-port
branch
from
0c29ce3 to
42d560d
Compare
| @@ -49,7 +49,7 @@ const DEFAULT_CONFIG: Partial<SnowpackConfig> = { | |||
| output: 'dashboard', | |||
| fallback: 'index.html', | |||
| hmrDelay: 0, | |||
| hmrPort: 12321, | |||
There was a problem hiding this comment.
Any reason to remove this? Whenever possible, it's good to give these defaults so that the rest of the codebase doesn't need to handle undefined values. So I'd vote to keep this here and remove DEFAULT_PORT
There was a problem hiding this comment.
If we set default hmrPort to 12321, we cannot distinguish between snowpack dev and snowpack dev --hmrPort=12321. Because we use port of devServer as hmrEngine's port while using snowpack dev, but we use 12321 as hmrEngine's port while using snowpack dev --hmrPort=12321.
There was a problem hiding this comment.
@FredKSchott There is another way to implement it. we can add a DEFAULT_BUILD_CONFIG in this file and merge DEFAULT_BUILD_CONFIG to config if snowpack is in build mode. The code of DEFAULT_CONFIG and DEFAULT_BUILD_CONFIG is as below.
const DEFAULT_CONFIG = {
// other default config
devOptions: {
hmrPort: undefined,
}
}
const DEFAULT_BUILD_CONFIG = {
devOptions: {
hmrPort: 12321,
}
}
I prefer this way since we usually need to set different configuration according to whether snowpack runs in dev mode or build mode.
I would like to use this way to modify this pr if you agree with it 😁.
There was a problem hiding this comment.
Ah right, of course. Okay, let's keep as it is now for this PR. If you'd like to create a follow up PR to simplify this config further, that would be great!
Changes
Part 2 of #1041.
The change is adding
window.HMR_WEBSOCKET_PORTin hmr-client.js file and only settingHMR_WEBSOCKET_PORTinproductionmode.Testing
manually test 5 cases:
yarn build --watch --hmr --hmrPort=15555. check settingwindow.HMR_WEBSOCKET_PORT=15555.yarn build --watch --hmr. check settingwindow.HMR_WEBSOCKET_PORT=12321.yarn build --watch --hmrand setwindow.HMR_WEBSOCKET_URLin html file. check thatwindow.HMR_WEBSOCKET_URLtakes precedence overwindow.HMR_WEBSOCKET_PORT.yarn build --watch. check no generating hmr code.yarn start. check no generating code of settingHMR_WEBSOCKET_PORT.yarn start --hmrPort=15555. check settingwindow.HMR_WEBSOCKET_PORT=15555and hmr server listens on 15555 port.