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

create-svelte: Directly depend on svelte-hmr #408

Closed
wants to merge 2 commits into from
Closed

Conversation

GrygrFlzr
Copy link
Member

The issue

Currently, the template produced by create-svelte does not directly depend on svelte-hmr, and instead brings it in as an indirect dependency via this chain:

devDependencies:
@sveltejs/snowpack-config 1.0.0-next.3
└─┬ @snowpack/plugin-svelte 3.5.2
  └── svelte-hmr 0.12.6

In reality, we are actually expecting the presence of svelte-hmr in node_modules, as proven when we try to use pnpm to install our dependencies:

$ pnpm init svelte@next
# no to typescript, use plain css
$ pnpm install
$ pnpm dev
> [email protected] dev C:\Users\GrygrFlzr\Documents\projects\pnpm-kit
> svelte-kit dev

[snowpack] ! building dependencies...
> Cannot find module 'svelte-hmr/runtime/hot-api-esm.js' from 'C:\Users\GrygrFlzr\Documents\projects\pnpm-kit'
Error: Cannot find module 'svelte-hmr/runtime/hot-api-esm.js' from 'C:\Users\GrygrFlzr\Documents\projects\pnpm-kit'
    at Function.resolveSync [as sync] (C:\Users\GrygrFlzr\Documents\projects\pnpm-kit\node_modules\.pnpm\r[email protected]\node_modules\resolve\lib\sync.js:102:15)

As described in the pnpm docs:

In most cases it means that one of the dependencies require packages not declared in package.json. It is a common mistake caused by flat node_modules. If this happens, this is an error in the dependency and the dependency should be fixed.

A user can of course work around this on their end by adding svelte-hmr themselves:

$ pnpm add -D svelte-hmr
 WARN  @sveltejs/snowpack-config > @snowpack/plugin-svelte: [email protected] requires a peer of rollup@>=2.0.0 but none was installed.
Already up-to-date
Progress: resolved 73, reused 73, downloaded 0, added 0, done

devDependencies:
+ svelte-hmr 0.12.6

$ pnpm dev

> [email protected] dev C:\Users\GrygrFlzr\Documents\projects\pnpm-kit
> svelte-kit dev

[snowpack] ! building dependencies...
[snowpack] ✔ dependencies ready! [0.39s]
  http://localhost:3001 • http://10.38.149.136:3001
  Server started in 438ms.


> Listening on http://localhost:3000

But I think it's better for the output produced by create-svelte to directly depend on svelte-hmr for the user out of the box.

@GrygrFlzr
Copy link
Member Author

Meta question: Do I generate a changeset with this PR, or is that left for Rich when the npm module is updated?

@Conduitry
Copy link
Member

You should probably include the changesets file in this PR. When Rich cuts a new release, he'll use those to form the basis for the changelogs for that version, and is unlikely to go hunting around in the commit log for other changes that might have happened but didn't add their own changesets.

@rixo
Copy link

rixo commented Feb 25, 2021

svelte-hmr is probably gonna stay its own package for some time, even if moved to the Svelte org.

I've had some time to reflect on this issue with pnpm since the first time I've been made aware of it...

I agree with the citation from pnpm's docs that the current state of things is a mistake that has been hidden by flat node_modules. However, in this case, I don't think exposing the dep to userland is the proper solution. IMO svelte-hmr is an implementation detail of the integration (i.e. the Svelte plugin), and should remain this way.

I now believe that if a transitive dep has some parts that needs to be accessible by the final consumer, the proper solution, conceptually, is for the intermediary dep to expose those parts through itself.

export { foo, bar, baz } from 'implicit-dep' // for the greater convenience of my user

That said, that doesn't fully apply to our case with svelte-hmr because here, the 2 problematic modules are runtime deps. So we shouldn't even assume Node resolution mechanism, or a file system (actually needs to be fixed in current svelte-hmr so that it can be used in a browser context, like a REPL).

Anyway, my point is only the integration (the plugin) that leverages svelte-hmr can have all the required information (the ability to locate the modules in question, which triggered this PR, but also paths vs URLs, how they will be resolved by the underlying bundler or browser, etc.), and so I believe that's where the fix belongs. The integration should tell svelte-hmr what import identifiers it should use for its runtime deps.

I was experimenting with svelte-hmr + pnpm just yesterday, and here's how I solved it:

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

I believe something similar should be made in the Snowpack plugin.

@GrygrFlzr
Copy link
Member Author

That makes sense yeah, especially since I can also replicate it with a Snowpack + non-Kit Svelte setup. I'll open an issue/discussion on the Snowpack repo.

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.

4 participants