-
-
Notifications
You must be signed in to change notification settings - Fork 423
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
Fix package.json files of dedupe-mixin and scoped-elements for proper module support #2648
base: master
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: b18c553 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
ccd53e0
to
751c562
Compare
@@ -14,6 +14,7 @@ | |||
"author": "open-wc", | |||
"homepage": "https://github.com/open-wc/open-wc/tree/master/packages/scoped-elements", | |||
"main": "index.js", | |||
"type": "module", |
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.
Pretty sure this will count as a breaking change.
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 don't think so. Bundlers look at the "exports"
without taking the "type"
into account.
NodeJS does take the "type"
into account, but fails as it tries to interpret the file as a CommonJS module.
So as far as I can see (and please correct me if I'm wrong, as different systems seem to do things differently and I want to know how to cover all bases) it's a bugfix, as it doesn't restrict anything, and fixes something that currently doesn't work when importing it from NodeJS.
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.
Im not sure if this is a breaking change, I think adding type: module is more "dangerous" for tooling packages/projects than packages like this, which is runtime/client only. As Jesse also mentioned, I don't think adding this changes anything in the way the module is resolved. I think we can ship this as a minor. @Westbrook do you agree?
"module": "index.js", | ||
"type": "module", | ||
"exports": { |
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.
Not 100% sure if I'd call this a breaking change, as it surfaces all the files that were previously available. However, maybe if should be and we should remove "./src/dedupeMixin.js": "./src/dedupeMixin.js"
and "./index.js"
as it's all reexported through "."
anyways?
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 is a breaking change, somebody could be importing without file extensions
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.
@Westbrook In #2496, it was mentioned that adding just the "."
export would make it a breaking change, after which the pull request starved. That's why I added the deep exports, so it wouldn't be a breaking change.
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.
@thepassle I've pushed an additional commit which adds the extensionless variants to the exports as well, so extensionless deep imports will also still work.
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.
We shouldnt support extensionless imports, because it bloats import maps
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.
makes total sense. got it!
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.
@thepassle comparing notes on this…
Agree it’s a breaking change. Do you have any issues with a breaking change here? As long as the PR is updated to be processed as such, it would seem that doing this is the right path for the broadest number of users, right?
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.
Im ok with a breaking change, its just important that we treat it as such :p I think we should probably align the package exports in the same way we do for scoped elements. The only thing im unsure about is why scoped elements has a “types.js” entry, when it already has a “types” export condition. @Westbrook do you happen to know why this is the case?
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've removed the named exports again, and turned the changeset into a major.
I also noticed another difference with scoped-elements: dedupe-mixin didn't have a "types"
entry in the package.json, which means it wouldn't work with TypeScript unless the "moduleResolution"
was set to either "node16"
, "nodeNext"
or "bundler"
, as it would then take the newly added "types"
in the "exports"
into account. I've added this as well.
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 is a broader ecosystem shift. for example, ttsc and ts-patch are broken on ts 5.0. ts-patch is working on a fix and presumably ttsc as well. I bring this up as food for thought when weighing out the value of a breaking change.
Personally, I think breaking user's imports is preferable to breaking the supported typescript ranges. In other words, if we can support both TS 4.9 and TS 5, I think that's work breaking user's imports, since those import path breakages are easy to update mechanically
What is the status of this? I am blocked because of this issue. |
Is there any progress on this? This is blocking us from moving to Vitest as well |
We are currently in the process of switching from Jest to Vitest. Vitest uses the built-in resolution system of NodeJS, which is stricter than that of bundlers. This causes issues with the
dedupe-mixin
andscoped-elements
packages.What I did
scoped-elements: Add
"type": "module"
to package.jsonWhen NodeJS imports a file, it determines which module system to use based on the extension:
*.cjs
files use CommonJS and*.mjs
use ES Modules. For*.js
files, it will pick a module system based on the package's"type"
field, defaulting to CommonJS. See also Determining module system in the NodeJS docs.This system works separately from the
"exports"
section in the package.json: if the referenced file from the exports block has a.js
extension and the package.json doesn't have a"type": "module"
, the file will be interpreted as a CommonJS module, even if it came from the"import"
condition in the"exports"
.This is the problem we're running into with the scoped-elements package: because the package.json is missing
"type": "module"
, Node imports the package as if it was a CommonJS module instead of an ES module, resulting in the following error:By adding the
"type": "module"
to the package.json, Node properly considers the file as an ES module.dedupe-mixin: Add "main" and "exports" to package.json
The package.json of dedupe-mixin only contains a
"module"
declaration, and no"main"
or"exports"
."module"
is non-standard, and only used by bundlers. This results in the following warnings when trying to import the package from NodeJS:To remove this warning, I've added
"main"
and"exports"
fields to the package.json.In another pull request I read that a concern about adding an
"exports"
field was that it would remove the possibility to do deep imports, which would make it a breaking change.To prevent this breaking change, I've also added "./index.js" and "./src/dedupeMixin.js" to the exports, so deep imports are still possible.