fix exports in package.json to work with ts's implementation of node16 moduleResolution #549
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,14 +23,19 @@ | |||||||||||
| ], | ||||||||||||
| "license": "Apache-2.0", | ||||||||||||
| "main": "dist/index.js", | ||||||||||||
| "types": "./dist/index.d.ts", | ||||||||||||
|
Comment on lines
25
to
+26
There was a problem hiding this comment. Are all the consumers of these packages updated? In which case we could keep things simple and just delete the
Suggested change
There was a problem hiding this comment. Yeah I was thinking about that. I don't know if I can tell 100% before just removing these. And if it doesn't "just work" I'd have to upgrade all our repos at once. I was thinking I can remove those in a separate PR and then if it all blows up I only have to revert that one PR. |
||||||||||||
| "exports": { | ||||||||||||
| ".": { | ||||||||||||
| "require": { | ||||||||||||
| "default": "./dist/index.js", | ||||||||||||
| "types": "./dist/index.d.ts" | ||||||||||||
|
There was a problem hiding this comment. I don't think anything that's going to use exports.require is actually going to pick up this types field because the commonjs moduleResolution in typescript was clearly happily using "types" from the root and now with node16 it will use import.types. plus node/js (outside of typescript) is never going to import the types anyway but it is here for consistency. There was a problem hiding this comment. A "types" condition like this is redundant, since TSC will automatically infer it from the .js file.
Suggested change
There was a problem hiding this comment.
is this the case? I have had issues with packages which don't explicitly define it before There was a problem hiding this comment. I guess that'd work if you do There was a problem hiding this comment. I'm not sure about this because if someone does use the commonjs module resolution method (or something in between) it would work if it picks both main and types from the root, but what if it picks exports.require? We'd basically rely on it falling back to the types field at the root? I don't think we want types inferred if there are proper types, do we? There was a problem hiding this comment. oh wait I think I'm misunderstanding. You mean the top-level one since ts will be using the exports anyway? I suspect you're right. There was a problem hiding this comment. Chatted with @lerouxb offline, I'd be happy to share an example for anyone interested. |
||||||||||||
| }, | ||||||||||||
|
Comment on lines
+29
to
+32
There was a problem hiding this comment. A "types" condition like this is redundant, since TSC will automatically infer it from the .js file.
Suggested change
There was a problem hiding this comment. so just for the "require" ones? I left a comment elsewhere where I already had a suspicion like that. |
||||||||||||
| "import": { | ||||||||||||
| "default": "./dist/.esm-wrapper.mjs", | ||||||||||||
| "types": "./dist/index.d.ts" | ||||||||||||
|
There was a problem hiding this comment. This comment here says that you need separate types files for require/import but so far in my testing by just npm linking things together it seems to work? What I'll do once this PR lands is to make sure we bump all these packages where they are used so that I find and fix any errors and not someone else. If we do need a types files in a different format, then I guess we'll have to change https://github.com/addaleax/gen-esm-wrapper |
||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| }, | ||||||||||||
| "scripts": { | ||||||||||||
| "bootstrap": "npm run compile", | ||||||||||||
| "prepublishOnly": "npm run compile", | ||||||||||||
|
|
||||||||||||
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 moved main and types together because logically that's how it works. Systems that import code will use those if they aren't using exports.