-
-
Notifications
You must be signed in to change notification settings - Fork 80
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
warn about missing svelte export condition #206
Conversation
index.js
Outdated
} | ||
} | ||
|
||
if (pkg) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a matter of style, but you could make this if (!pkg) return;
. The whole if
block doesn't fit on one screen so it's hard to see at a glance if there's anything below it, but if you invert the condition it's easy to tell what happens
index.js
Outdated
resolve(pkg, entry, { conditions: ['svelte'] }); | ||
|
||
if (!warned) { | ||
console.error('\n\u001B[1m\u001B[31mWARNING: Your @rollup/plugin-node-resolve configuration\'s \'exportConditions\' array should include \'svelte\'. See https://github.com/sveltejs/rollup-plugin-svelte#svelte-exports-condition for more information\u001B[39m\u001B[22m\n'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we first say what it was that we failed to resolve? It might be a better message in the case that resolving fails for any other reason. I don't have any other reason in mind, but it might protect against something unexpected happening
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the message is fine as is, I don't see a situation where it would be wrong.
} | ||
|
||
const resolved = await this.resolve(importee, importer, { skipSelf: true }); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should check here if svelte
and default
point different places and we resolved to default
then we should print a warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't happen for the foreseeable future (almost no library will run into this), so I'm in favor of merging now to get it out.
Co-authored-by: Ben McCann <[email protected]>
We plan to release a version of
svelte-package
that encourages package authors to omit thedefault
export condition. To resolve these packages, Rollup users are required to haveexportConditions: ['svelte']
in their@rollup/plugin-node-resolve
config.With this change, we can identify cases where the field is not configured, and print a loud warning that will hopefully allow people to self-diagnose resolution failures and fix them.