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

Installing app dependencies with pnpm results in svelte-hmr not being found #444

Closed
Conduitry opened this issue Mar 4, 2021 · 15 comments
Closed
Labels
bug Something isn't working

Comments

@Conduitry
Copy link
Member

Conduitry commented Mar 4, 2021

npm create svelte@next foo
cd foo
pnpm install
npm run dev

Visiting http://localhost:3000 results in a console error about [vite] Internal server error: Failed to resolve import "svelte-hmr/runtime/hot-api-esm.js". Does the file exist? and a non-interactive app in the browser. This is presumably because pnpm isn't hoisting svelte-hmr like npm is, and something is incorrectly assuming it has transitive access to that package. This works fine when installing dependencies with npm.

pnpm 5.18.1, node 15.11.0.

@Conduitry Conduitry added the bug Something isn't working label Mar 4, 2021
@Conduitry
Copy link
Member Author

I had had this happen before in #103, but I'm pretty sure this is a different issue. There, I discovered I had been running into nodejs/node#22648 but I no longer have cap_net_bind_service set on my Node binary.

@dummdidumm
Copy link
Member

dummdidumm commented Mar 4, 2021

Related #408 (contains the possible reason, pnpm being strict about dependencies)

@Conduitry
Copy link
Member Author

I can reproduce this one from a fresh node:alpine Docker container, so I'm pretty sure this is no longer related to something weird and environmental on my machine.

@Conduitry
Copy link
Member Author

Ah, there it is. I thought I had seen an issue that sounded related to this sometime recently. I think I couldn't find it because I wasn't looking for a PR.

@Conduitry
Copy link
Member Author

Conduitry commented Mar 4, 2021

Reading that closed PR, it sounds like this would be something to handle in the Vite Svelte plugin then? Whatever corresponds to the Snowpack code at https://github.com/snowpackjs/snowpack/blob/d35d50b4c1696b9ef17ad9aa626154cb6c82fb83/plugins/plugin-svelte/plugin.js#L8 ? edit: Presumably, the corresponding code would be

const makeHot = createMakeHot({ walk });
edit again: hacking the same change that @rixo mentioned in that other PR right into node_modules/.pnpm/@sveltejs/[email protected][email protected][email protected]/node_modules/@sveltejs/vite-plugin-svelte/index.js does seem to make this work.


What's the difference currently between @sveltejs/vite-plugin-svelte and the official Vite plugin? I remember seeing some mention of that somewhere recently, but didn't see whether there was any conclusion.

@benmccann
Copy link
Member

benmccann commented Mar 4, 2021

Vite has a plugin hook handleHotUpdate that's called for hot updates https://vitejs.dev/guide/api-plugin.html#handlehotupdate

I believe Rich was interested in making the plugin a bit more official and possibly having it live under @sveltejs though Dominik was hesitant to have it live in a closed repo. Maybe we can revisit the discussion after the repo opens up to figure out what the plans should be. Anyway, Rich switched the code yesterday to use the svite version instead of the @sveltejs version because he wanted types, which were only provided by the svite version.

I guess this is the change you're talking about from @rixo? #408 (comment)

@antony
Copy link
Member

antony commented Mar 4, 2021

FWIW I ran into this today, using a fresh vite project and then adding svelte to it. Also with pnpm.

@rixo
Copy link

rixo commented Mar 4, 2021

I believe the code snippet in the other issue was a incorrect.

I think it should be:

makeHot = createMakeHot({
  walk,
  adapter: relative.resolve('svelte-hmr/runtime/proxy-adapter-dom.js', __dirname),
  hotApi: relative.resolve('svelte-hmr/runtime/hot-api-esm.js', __dirname),
})

We need to resolve from a file / directory of the Vite plugin itself, not cwd, because svelte-hmr is a direct dep of the plugin, and so that's from the plugin's directory that it is resolvable with node mechanism.

@Rich-Harris Rich-Harris added this to the public beta milestone Mar 5, 2021
@rixo
Copy link

rixo commented Mar 5, 2021

After further investigation, I think the approach I suggested earlier, of exposing correct entry points from the svelte-hmr package for CJS and ESM, is a step in the right direction but for Kit it won't suffice.

Since Kit is bundling the Vite plugin itself, there seems to be no good way for svelte-hmr or the plugin to resolve runtime paths into svelte-hmr. Kit will have to do it itself and pass them down to svelte-hmr through the plugin's options.

At this point, it would seem only consistent for Kit to also bundle svelte-hmr and ship it as its own runtime. It would be weird to bundle everything and keep one lone package (a sensitive one at that, as we've seen) that could down Kit from upstream with a rogue version.

That should be something like runtime/app/internal/hmr.js, I guess?

I'm digging this later tonight.

@rixo
Copy link

rixo commented Mar 6, 2021

I apparently got confused about Kit bundling the Vite plugin together with its cli... So in this case the fix should simply be for svelte-hmr to resolve absolute paths to its runtime deps and inject those in the transformed components code. Vite2 should be ok with absolute paths (Vite1 was not).

We have done this in the current pre-releases of svelte-hmr and @svitejs/vite-plugin-svelte, and so this issue should now be fixed @svitejs/[email protected]. It seems to be the case on my machine with symlinked packages (not tested Windows yet), but I can't confirm because right now running the exact same commands as @Conduitry now gives me a different error.

In a node:alpine docker container, with node 15.11.0 and pnpm 5.18.3, running this:

npm create svelte@next foo
cd foo
pnpm install
npm run dev

Gives me this:

~/foo # npm run dev

> [email protected] dev
> svelte-kit dev

> Listening on http://localhost:3000
Error: No element indexed by 0
    at ArraySet$2.ArraySet_at [as at] (/root/foo/node_modules/.pnpm/[email protected]/node_modules/vite/dist/node/chunks/dep-e0f09032.js:24278:9)
    at BasicSourceMapConsumer.<anonymous> (/root/foo/node_modules/.pnpm/[email protected]/node_modules/vite/dist/node/chunks/dep-e0f09032.js:25193:67)
    at Array.map (<anonymous>)
    at BasicSourceMapConsumer.SourceMapConsumer_eachMapping [as eachMapping] (/root/foo/node_modules/.pnpm/[email protected]/node_modules/vite/dist/node/chunks/dep-e0f09032.js:25192:14)
    at merge (/root/foo/node_modules/.pnpm/[email protected]/node_modules/vite/dist/node/chunks/dep-e0f09032.js:26656:18)
    at ssrTransform (/root/foo/node_modules/.pnpm/[email protected]/node_modules/vite/dist/node/chunks/dep-e0f09032.js:61382:15)
    at transformRequest (/root/foo/node_modules/.pnpm/[email protected]/node_modules/vite/dist/node/chunks/dep-e0f09032.js:61650:48)
    at async instantiateModule (/root/foo/node_modules/.pnpm/[email protected]/node_modules/vite/dist/node/chunks/dep-e0f09032.js:67994:10)

No idea what this means.

Same if I install deps with npm instead of pnpm.

@dummdidumm
Copy link
Member

Sounds like #424

@rixo
Copy link

rixo commented Mar 6, 2021

@dummdidumm Yes, it does. Thanks for the pointer. Apparently happens because I picked TS in the cli menu. Adding a $layout.svelte to the project fixes it.

So! By picking no TS, I was able to reproduce the original issue, and using a linked Kit that uses @svitejs/[email protected] fixes it. So I think we're good on the svelte-hmr + pnpm issue.

@benmccann
Copy link
Member

Does that mean we should close this issue? (and maybe #424)

@Conduitry
Copy link
Member Author

This should only be closed if we've updated to the version of the Vite plugin that fixes the svelte-hmr thing.

@benmccann
Copy link
Member

We should be. I upgraded it 0.10.0-0 this morning. Probably not released yet though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants