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

Resolve all imports from svelte/... to the same copy #58

Closed
Conduitry opened this issue Jun 9, 2019 · 6 comments
Closed

Resolve all imports from svelte/... to the same copy #58

Conduitry opened this issue Jun 9, 2019 · 6 comments

Comments

@Conduitry
Copy link
Member

As mentioned here npm linking in a package containing a Svelte component can cause issues with two copies of the Svelte internals, because imports from svelte/internals will resolve to that linked package's node_modules/svelte.

One way to address this is to compile with sveltePath: '\0svelte' (to prevent rollup-plugin-node-resolve from trying to resolve these modules, even if its resolveId hook runs before ours), and then to manually resolve those imports in our resolveId hook.

@Conduitry
Copy link
Member Author

A potential issue: Imports from svelte from outside Svelte components. I can't tell when this might be an issue. It seems likely to cause problems with encapsulated lifecycle hook stuff.

We could resolve this by going back to the first idea I had - just manually resolving all svelte/ stuff in this plugin's resolveId hook, and hope that we're before rollup-plugin-node-resolve.

@Conduitry
Copy link
Member Author

@mrkishi suggested using the options plugin hook to unshift a special resolveId hook to the array of plugins, so that will always happen before rollup-plugin-node-resolve. I'm not sure what version of Rollup that was added in though. I haven't found its addition in the changelog. This plugin currently requires Rollup 0.60+, so any requirements beyond that would be a breaking change.

@Conduitry
Copy link
Member Author

Never mind, I couldn't find it because I stopped looking before I got to it. rollup/rollup#371 This was added ages ago.

@mrkishi
Copy link
Member

mrkishi commented Jun 9, 2019

0.60 had the options hook already, but isn't this change by definition breaking even without the prioritized resolution? Even though people shouldn't be relying on multiple copies, it is new behavior.

Now, adding the resolve to the top of the plugins is definitely breaking to me :P

@Conduitry
Copy link
Member Author

Initial hacky implementation:

diff --git a/index.js b/index.js
index 330e8b9..14555e9 100644
--- a/index.js
+++ b/index.js
@@ -7,6 +7,8 @@ const { encode, decode } = require('sourcemap-codec');
 
 const major_version = +version[0];
 
+const home_svelte_path = require.resolve('svelte/package.json').slice(0, -19);
+
 const { compile, preprocess } = major_version >= 3
 	? require('svelte/compiler.js')
 	: require('svelte');
@@ -165,6 +167,9 @@ module.exports = function svelte(options = {}) {
 		},
 
 		resolveId(importee, importer) {
+			if (importee === 'svelte' || importee.startsWith('svelte/')) {
+				return this.resolve(home_svelte_path + importee);
+			}
 			if (cssLookup.has(importee)) { return importee; }
 			if (!importer || importee[0] === '.' || importee[0] === '\0' || path.isAbsolute(importee))
 				return null;

I was running into some weird errors when I was trying to use the options hook. I'll try to look into this a little more later.

I was also running into some weird errors when I was linking a local copy of rollup-plugin-svelte into an app. I tested the above changes by directly editing node_modules/rollup-plugin-svelte/index.js.

@Conduitry
Copy link
Member Author

This has been fixed in the Svelte and Sapper templates by using the new dedupe option of rollup-plugin-node-resolve - there's no need for this to be handled in this plugin anymore. Closing.

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

No branches or pull requests

2 participants