Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#844 fix for SSL HMR websockets #1517

Merged
merged 10 commits into from
Jun 3, 2021
Merged

#844 fix for SSL HMR websockets #1517

merged 10 commits into from
Jun 3, 2021

Conversation

JBusillo
Copy link
Contributor

This is a proposed fix to Issue #844

Currently, when the dev server is invoked using the https option (-H or --https) using an unsigned certificate, the Vite HMR web client cannot connect to the server using its 'wss' url. It retries incessantly. Browsers will reject the connection, as the user can't 'approve' the possibly unsafe site.

To correct this, the web socket should 'piggy-back' onto the https server. Thus, when the user selects 'Proceed' from the 'Your connection is not private' dialog, both https and wss connections will be deemed valid. Vite manages the wss connection upgrade from this point forward.

The get_server function must be called before vite.createServer, since the server reference must be available to vite.createServer.

Perhaps I'm being too cautious, but I didn't want to switch the order of the current vite.createServer block (of code) with the get_server block. The dev server isn't ready to process requests until vite.createServer is complete. Thus, you'll see that I coded a dummy handler that is eventually replaced once vite.createServer has completed.

I ran the tests within kit (pnpm run test), and ran tests on both 'dev' and 'preview' with all combinations of the --https and --host options.

This is my first pull request ever -- please don't be too hard on me.

@JBusillo
Copy link
Contributor Author

Is it normal if the netlify tests were cancelled? There were no changes in the output build, since this change only affects dev and preview, when the -H or --host parameters are supplied.

@benmccann
Copy link
Member

don't worry about the Netlify tests. I think they only run for changes to the Netlify adapter

I was having a little bit of a hard time following your explanation, but I would say to go ahead and switch the order. Putting in a dummy handler and then switching it out later make it harder to follow the code. It also seems to me like it'd make things less likely to work as expected because what if a request comes in while the dummy handler is installed

@JBusillo
Copy link
Contributor Author

Good idea. Code changed, tested locally in dev & preview using demo app. I think I was just being too cautious -- not wanting to reorder code.

@JBusillo JBusillo requested a review from benmccann June 2, 2021 23:52
@JBusillo JBusillo requested a review from benmccann June 3, 2021 04:36
Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for being patient with this one and following up with the Vite team about getting it documented

@benaltair
Copy link

benaltair commented Jun 17, 2021

@benmccann a few people on discord are reporting identical errors after the 114 update. It's been reported that switching back to 113 has fixed it. Wondering if this PR was the culprit? Here are my Netlify build logs with 115 for instance:

8:27:12 PM: > The requested module 'svelte-hmr' does not provide an export named 'createMakeHot'
8:27:12 PM: file:///opt/build/repo/node_modules/@sveltejs/vite-plugin-svelte/dist/index.js:214
8:27:12 PM: import { createMakeHot } from "svelte-hmr";
8:27:12 PM:          ^^^^^^^^^^^^^
8:27:12 PM: SyntaxError: The requested module 'svelte-hmr' does not provide an export named 'createMakeHot'
8:27:12 PM:     at ModuleJob._instantiate (internal/modules/esm/module_job.js:92:21)
8:27:12 PM:     at async ModuleJob.run (internal/modules/esm/module_job.js:107:20)
8:27:12 PM:     at async Loader.import (internal/modules/esm/loader.js:179:24)
8:27:12 PM:     at async file:///opt/build/repo/node_modules/@sveltejs/kit/dist/cli.js:647:22

Messages:

Here's the update branch on my repo that was failing builds on Netlify:
https://github.com/bahaistudies/conference/tree/update-115

Happy to provide more details or open an issue if you think it's needed.

@benaltair
Copy link

I tried downgrading to Kit 113 on its own, and still got the error. The error went away when I downgraded Vite to 2.3.6
Here's my package.json for reference, now with it working: https://github.com/bahaistudies/conference/blob/6004f98314d4bbeca399f45f2880190c559f1fee/package.json

@benmccann
Copy link
Member

Hmm. Perhaps @rixo or @dominikg know something about svelte-hmr's involvement or @GrygrFlzr knows anything about recent changes in Vite

@JBusillo
Copy link
Contributor Author

JBusillo commented Jun 17, 2021

I cloned your repro -- dev, build and preview work with SvelteKit 1.0.0-next.115 and Vite 2.3.7. However "start" has been renamed to "preview", which tells me that you haven't updated your package.json, and possibly some other configuration(s) that have breaking changes since your previous SvelteKit version.

You might want to create a new SvelteKit demo project, and compare package.json, svelte.config.js, and any other config files, along with your library structure.

#844 only affects dev and preview modes. Also, Netlify deployments shouldn't even reference HMR. To be honest, I don't know anything about Netlify.

@benaltair
Copy link

benaltair commented Jun 17, 2021

Thanks for the tips @JBusillo. I've gotten it to run by first adopting all the config file changes, then I needed to add .node-version in the root with the contents 14.13.1 because otherwise I was getting an error from Netlify that the default version of Node they use is not compatible with the requirements in Kit. https://github.com/bahaistudies/conference/commit/5c13d2d7040fcb7d9510e9a805a44023b4ddbcc6

11:20:20 PM: npm ERR! code ENOTSUP
11:20:20 PM: npm ERR! notsup Unsupported engine for conference@0.0.1: wanted: {"node":"^14.13.1"} (current: {"node":"12.18.0","npm":"6.14.4"})
11:20:20 PM: npm ERR! notsup Not compatible with your version of node/npm: conference@0.0.1
11:20:20 PM: npm ERR! notsup Not compatible with your version of node/npm: conference@0.0.1
11:20:20 PM: npm ERR! notsup Required: {"node":"^14.13.1"}
11:20:20 PM: npm ERR! notsup Actual:   {"npm":"6.14.4","node":"12.18.0"}

If this isn't an isolated issue, it might be worth adding a FAQ to the Netlify adapter that this file is needed, or even just add it to the skeleton project if it doesn't cause any harm perhaps?

Regardless, after this troubleshooting and that extra step for the Node version, it's now working for me.

@benmccann
Copy link
Member

The adapter should be able to spit out a .node-version or .nvmrc file as well. I'm not sure whether the better approach is adding it there or to the skeleton

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants