-
Notifications
You must be signed in to change notification settings - Fork 7
chore: Use type: "module" and build to cjs/mjs #190
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #190 +/- ##
=======================================
Coverage 99.05% 99.05%
=======================================
Files 42 42
Lines 1165 1165
Branches 317 316 -1
=======================================
Hits 1154 1154
Misses 10 10
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2d8526a to
a951be2
Compare
| ], | ||
| "no-warning-comments": "warn" | ||
| "no-warning-comments": "warn", | ||
| "import/extensions": ["error", "ignorePackages"] |
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 need ".js" extensions so that mjs imports are resolved correctly. Without that, the downstream builds fail. The same issue exists in the collection-hooks. As a follow-up change, I plan to apply the same changes to the collection-hooks eslint config.
5ce65ae to
1cc1b31
Compare
| }, | ||
| "sideEffects": false, | ||
| "types": "./mjs/index.d.ts", | ||
| "typesVersions": { |
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 needed to fix downstream packages that import from /dom or /internal. That will not be needed should we update the packages to use moduleResolution: "nodenext".
5797563 to
e03e17d
Compare
2e70409 to
a16e542
Compare
| packageCjs.type = 'commonjs'; | ||
|
|
||
| writeFileSync(libPkgPath, JSON.stringify(packageCjs, null, 2) + '\n'); | ||
| writeFileSync(esmPkgPath, JSON.stringify({ type: 'module' }, null, 2) + '\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.
Is this writing just {type: module} as the entire file content?
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.
What are these two additional package.json files actually for?
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 only publish the /lib folder - that's why the /lib/package.json is needed - it describes the package. However, as the /lib includes cjs exports, we have to change the type from "module" to "commonjs" after copying it.
In /lib/mjs we have ejs exports, therefore type "module" is needed again. There is no need to copy the entire package.json though - it will extend from the one in the package's root.
Required for: cloudscape-design/collection-hooks#130
Requires:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.