-
Notifications
You must be signed in to change notification settings - Fork 68
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
Support Node16 ESM resolution #140
Conversation
Also it should closes #113 |
|
Note: multi-input bundle output is broken on rollup@v3 |
decb0da
to
075edb7
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Any chance of getting merged? maybe it should be the next major? |
4ea96b9
to
9b0501b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Looks good! Would also help with #147. |
@arcanis cleaned up to be minimal changeset to support NodeJS/TypeScript ESM |
6a501e6
to
aea3797
Compare
"./platform": { | ||
"node": { | ||
"types": "./lib/platform/node.d.ts", | ||
"require": "./lib/platform/node.js", | ||
"import": "./lib/platform/node.mjs" | ||
}, | ||
"browser": { | ||
"types": "./lib/platform/browser.d.ts", | ||
"require": "./lib/platform/browser.js", | ||
"import": "./lib/platform/browser.mjs" | ||
} | ||
}, |
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.
It's not supposed to be exposed to consumers and bundlers should follow the browser
field if needed.
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.
The browser field exists for some legacy bundlers and is not supported by the whole ecosystem widely. eg. TypeScript
@@ -1,10 +1,10 @@ | |||
import * as platform from 'clipanion/platform'; |
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 change shouldn't be needed.
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 the node's native way, compatible with the whole ecosystem, including Node, TS, and modern bundlers
"@types/jest": "^29.5.9", | ||
"@types/lodash": "^4.14.179", | ||
"@types/node": "^14.0.13", | ||
"@typescript-eslint/eslint-plugin": "^5.43.0", | ||
"@typescript-eslint/parser": "^5.43.0", | ||
"@types/rollup": "^0.54.0", |
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.
Rollup includes its own types.
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.
It's rollup v2
I added This is currently the only configuration that is compatible with |
This is re-trying PR from #137
Tested on Node v16 with TypeScript v3.8, both CommonJS and ESM
node index.mjs test
: OKnode index.cjs test
OKNote this PR will drop support for Node v14