-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
NodeNext resolution failed to resolve dual-package correctly #50466
Comments
Okay, I think I'm starting to see why @weswigham is so insistent on I believe that what's going wrong here has to do with how the package interprets a As a result, a If you set "exports": {
".": {
"import": {
"types": "./dist/module-type.d.mts",
"default": "./dist/module.mjs"
},
"require": {
"types": "./dist/commonjs-type.d.cts"
"default": "./dist/common.cjs",
}
}
} The docs may need to be updated in light of this. |
This is not just a documentation problem. In the case I provided (which I believe is very common in the ecosystem), ESM and CommonJS have the same export list under two different import modes. // test.cjs
const f = require('testpkg')
f() // f is callable.
// test.mjs
import f from 'testpkg'
f() // f is also callable. |
@Jack-Works your export map is essentially telling TS to load both your esm and cjs entry point as cjs, since it's only providing a cjs format types file. You must have a separate TS file (with matching format) for each, as @DanielRosenwasser says, if you want accurate checking. |
I am one of the maintainers of a "dual-package" and after reading this thread I have some questions related to the thread comments:
More detailed below: The package in question is declared as ES Module (the
In this case, both the CJS and the ESM implementation expose the exact same API (actually the CJS implementation is transpiled from the ESM one via Rollup inside a compiler). That's how the exports look like for each implementation: # someFile.js (ESM)
export default class C {} # someFile.cjs (CJS)
class C {}
module.exports = C; That's how the type declaration looks like: export default class C {} And that's how we configured the
As soon as TS 4.8 has been released I tried to switch I am also asking as the type declaration is being generated from our compiler and I will have to think about how we can generate one for CJS if we have to. For instance, while trying to use the package in a project that uses CommonJS I am getting the following error from
Thanks in advance for your input. |
Yes. "./*": {
"import": {
"types": "./dist/*.d.ts",
// ~~
"default": "./dist/*.js"
},
"require": {
"types": "./dist/*.d.cts",
// ~~~
"default": "./dist/*.cjs"
}
} This will fix the problem. You should emit double declaration files (CJS once, and ESM once) with different extension names to make things work (which I think is very incomprehensible). If all of your imports can be imported in both CJS and ESM mode and set correctly under NodeNext resolution, I think you can just copy the file and rename it to I tried to set-up a dual package correctly (run tsc twice to emit both https://github.com/Jack-Works/ts-dual-package-example but it's very crazy. |
@weswigham @andrewbranch do you confirm that's what we should be doing? Just want to make sure I am getting this right. On a side note, do you mind giving pointers in which part of the TS codebase that resolution process is implemented? |
All module resolution is in |
Thx for your answers @weswigham and @Jack-Works. I think my confusion come from the latest code snippet from this page of the handbook: https://www.typescriptlang.org/docs/handbook/esm-node.html#packagejson-exports-imports-and-self-referencing
// package.json
{
"name": "my-package",
"type": "module",
"exports": {
".": {
// Entry-point for TypeScript resolution - must occur first!
"types": "./types/index.d.ts",
// Entry-point for `import "my-package"` in ESM
"import": "./esm/index.js",
// Entry-point for `require("my-package") in CJS
"require": "./commonjs/index.cjs",
},
},
// CJS fall-back for older versions of Node.js
"main": "./commonjs/index.cjs",
// Fall-back for older versions of TypeScript
"types": "./types/index.d.ts"
} Which seems to contradict what is being discussed here. Should the documentation be updated? |
Yes. |
Thanks for clarifying this @weswigham! I have a last confusion to clear up regarding around the Basically I am wondering if that type declaration: // someFile.d.ts
import {Base as _Base } from '@scope/runtime';
import A from 'package/paths/a';
export default class Foo extends _Base {
getFoo(): A
} Could be ported to // someFile.d.cts
import {Base as _Base } from '@scope/runtime';
import A from 'package/paths/a';
declare class Foo extends _Base {
getFoo(): A
}
exports = Foo; |
The correctness of any declaration is determined by the implementation file it types, so you can’t answer that question without looking at the |
Thanks, @andrewbranch, it makes sense. I guess my question is more related to the syntax one should be used for exports/imports in |
I’m still not sure the question totally makes sense—to ask whether any declaration file should use this or that syntax is just to ask what syntax exists in its implementation file counterpart. There’s never a case where a declaration file is designed from a blank slate and then an implementation follows. In the simplest cases the declaration file and the implementation file will both be generated simultaneously from a TS source file by tsc. The only time we need to think about writing a declaration file by hand is when typing an external or native dependency, as contributors to DefinitelyTyped do. But in those cases, we have a reference implementation or API to match, so the question of what should go in the declaration file is objectively answerable by looking at the implementation. In your case Rollup should be handling all of this for you, right? Are you trying to figure out if Rollup is doing the right thing? |
Our use case is a bit specific so let me try to clarify. My team works on a source-to-source compiler that's generating JavaScript (in both CJS & ESM) and a type declaration (.d.ts) from a single JSON file. So for every matching JSON file, it generates the following files:
The That's the structure of the generated JS for the ESM module format: // file.js
import {Base as _Base } from '@scope/runtime';
import A from 'package/paths/a';
export default class Foo extends _Base {
/**
* @returns {A}
*/
getFoo() {
// impl...
return new A();
}
} That's the structure of the generated JS for the CommonJS module format: // file.cjs
var core = require('@scope/runtime');
var _A = require('package/paths/a');
// Interop fn added by Rollup
function _interopDefaultLegacy (e) { return e && typeof e === 'object' && 'default' in e ? e : { 'default': e }; }
var _A__default = /*#__PURE__*/_interopDefaultLegacy(_A);
class Foo extends core.Base{
/**
* @returns {A}
*/
getFoo() {
// impl...
return new _A__default['default']();
}
}
module.exports = Foo; That's what the generated type declaration looks like: // file.d.ts
import {Base as _Base } from '@scope/runtime';
import A from 'package/paths/a';
export default class Foo extends _Base {
getFoo(): A
} We don't rely on We are using that compiler to generate a bunch of files packaged in an npm package. I updated the I've tried running // file.d.ts
export default class C{} Becomes: // file.d.cts
declare class C{}
export = C; And I am trying to see what I should do about the imports: import {Base as _Base } from '@scope/runtime';
import A from 'package/paths/a'; And eventually, port that plugin to Rollup and back it up in our compiler transpilation stage. |
Because the |
Thx a lot @andrewbranch, I really appreciate your explanations on this. My question was related to the import in the |
You can’t generally assume that an import in an ESM file will resolve the same when you convert that file to a CJS context. This is by design, of course; you’re taking advantage of conditional exports yourself. If you leave the imports untouched, TypeScript will resolve them “correctly,” but will it resolve to the same thing as it did in an ESM context? If not, will the import syntax used be appropriate for the different module it resolves to? Will the resulting file type check and do what you expect? You have to answer these questions yourself for every individual import. |
That makes sense, thanks a lot for all the very useful information, I can see what I should be doing now! |
People are writing the wrong package.json again and again in the wild. Example: react-hook-form/resolvers#460 |
new example: krisk/Fuse#692 |
I think TypeScript should warn when a |
To add to the dialog: It took ages to find this solution in order to get dual package exports working properly for TypeScript, but this solution by @Jack-Works works for us 🎉 (with some "gotchas" mentioned below). Our package.json for our ESM "library" looks like: {
"name": "library",
"type": "module",
"exports": {
"./foo": {
"import": {
"types": "./dist/foo.d.ts",
"default": "./dist/foo.js"
},
"require": {
"types": "./dist/foo.d.cts",
"default": "./dist/foo.umd.cjs"
}
},
"./bar": {
"import": {
"types": "./dist/bar.d.ts",
"default": "./dist/bar.js"
},
"require": {
"types": "./dist/bar.d.cts",
"default": "./dist/bar.umd.cjs"
}
}
}
}
Then, we are able to have our CJS "consumer" use this ESM "library" with the following {
"compilerOptions": {
"target": "ES6",
"module": "NodeNext"
}
}
Here is the kicker: Simply copying the // @file foo.ts
export { util } from './util.js';
When
The fix: rewrite all import { readFileSync, writeFileSync } from 'node:fs';
import { globby } from 'globby';
const paths = await globby(['dist/**/*.ts']);
for (const esmPath of paths) {
const esmContent = readFileSync(esmPath, { encoding: 'utf8' });
// Fix file extensions
const cjsPath = esmPath.replace('.d.ts', '.d.cts');
// Fix import/export references
const cjsContent = esmContent.replace(/(.+ from )'(.+).js'/g, "$1'$2.cjs'");
writeFileSync(cjsPath, cjsContent);
} This feels very hacky and we would absolutely like to ditch this completely - but it seems to be the only way to achieve the following:
We would ❤️ to see more native support that doesn't require so much juggling. |
Got same problem here Provided solution works fine, but be sure that consumer code that using your library is in CJS Forcing CJS target in consumer code looks like a little strange because docs said that choosing nodenext is the only right option to void such problems and nodenext supports CJS libs =/ |
Bug Report
NodeNext resolution failed to resolve dual-package correctly
🔎 Search Terms
NodeNext
🕗 Version & Regression Information
💻 Code
https://github.com/Jack-Works/ts-nodenext-wrong-resolution-reproduction
where
node_modules/testpkg/package.json
isTypeScript should resolve
type.d.ts
in dual mode instead of CommonJS synthetic export.🙁 Actual behavior
🙂 Expected behavior
No error
The text was updated successfully, but these errors were encountered: