Initial support for module: node12 #44501
Conversation
|
@typescript-bot pack this |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at 00d3e60. You can monitor the build here. |
|
Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@typescript-bot pack this |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at 632b495. You can monitor the build here. |
|
@typescript-bot test this |
|
Heya @weswigham, I've started to run the extended test suite on this PR at 632b495. You can monitor the build here. |
|
Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@typescript-bot test this |
|
Heya @weswigham, I've started to run the extended test suite on this PR at 632b495. You can monitor the build here. |
| const parts = getPathComponents(fileName); | ||
| parts.pop(); | ||
| while (parts.length > 0) { | ||
| const pkg = getPackageJsonInfo(getPathFromPathComponents(parts), /*onlyRecordFailures*/ false, state); | ||
| if (pkg) { | ||
| return pkg.packageJsonContent?.type === "module" ? ModuleKind.ESNext : ModuleKind.CommonJS; | ||
| } | ||
| parts.pop(); | ||
| } |
There was a problem hiding this comment.
Any reason not to use forEachAncestorDirectory?
| return undefined; | ||
| } | ||
| function lookupFromPackageJson(): ModuleKind.ESNext | ModuleKind.CommonJS { | ||
| const state: { |
There was a problem hiding this comment.
Nit: if getPackageJsonInfo is going to be exported, it might be nice if its parameter types (ModuleResolutionState) were too. Alternatively, Parameters<typeof getPackageJsonInfo>[2]?
| // Though an error in es2020 modules, in node-flavor es2020 modules, we can helpfully transform this to a synthetic `require` call | ||
| // To give easy access to a synchronous `require` in node-flavor esm. We do the transform even in scenarios where we error, but `import.meta.url` | ||
| // is available, just because the output is reasonable for a node-like runtime. | ||
| return getEmitScriptTarget(compilerOptions) >= ModuleKind.ES2020 ? visitImportEqualsDeclaration(node as ImportEqualsDeclaration) : undefined; |
There was a problem hiding this comment.
Maybe the comment is trying to explain this, but I’m not getting it: why wouldn’t we make this dependent upon a node12+ module rather than the script target? This isn’t going to produce anything useful for browser-bound ES2020 modules, right?
There was a problem hiding this comment.
Well, we'll issue an error on this import form in non-node es2020+, we're just choosing to emit the node-style transpilation, rather than omit the erroneous node, since the node-style transpilation is still pretty reasonable (and could actually work on other runtimes if they're providing a cjs interop layer).
There was a problem hiding this comment.
Got it. I really like the createRequire helper, by the way. That’s kind of a pain in vanilla JS.
| @@ -6099,7 +6119,11 @@ namespace ts { | |||
| // module kind). | |||
| ES2015 = 5, | |||
| ES2020 = 6, | |||
There was a problem hiding this comment.
Just to make sure I have the distinction correct: ES2015–ESNext are the correct setting for ESM in browsers, right? And possibly in other non-Node JS runtimes (but those are likely to layer their own ideas on top, like Node)? But we lack a corresponding module resolution mode that actually does the right thing for them?
There was a problem hiding this comment.
Yeah. We don't have a module resolution mode that understands URLs at all, so classic mode is as close as we get.
There was a problem hiding this comment.
As an example/datapoint, GJS (JavaScript for GNOME) supports ESM so our projects would likely use the ES2015-ESNext module settings.
We support either a root-level command line flag (-m/--module), a root-level API (gjs_evaluate_module), or treat all dynamic imports (import('./x.js')) as ESM (regardless of extension).
src/compiler/checker.ts
Outdated
| @@ -7141,8 +7168,10 @@ namespace ts { | |||
| A93C | if (!target) { | ||
| return; | |||
| } | |||
| let verbatimTargetName = unescapeLeadingUnderscores(target.escapedName); | |||
| if (verbatimTargetName === InternalSymbolName.ExportEquals && (compilerOptions.esModuleInterop || compilerOptions.allowSyntheticDefaultImports)) { | |||
| // If `target` refers to a shorthand module symbol, the name we're trying to pull out isn;t recoverable from the target symbol | |||
There was a problem hiding this comment.
This is the drive-by fix, right? What’s a shorthand module symbol?
| // If `target` refers to a shorthand module symbol, the name we're trying to pull out isn;t recoverable from the target symbol | |
| // If `target` refers to a shorthand module symbol, the name we're trying to pull out isn't recoverable from the target symbol |
There was a problem hiding this comment.
It's a (member of a) declare module "foo"; defined module.
src/compiler/program.ts
Outdated
| * `options` parameter. | ||
| * | ||
| * @param fileName The normalized absolute path to check the format of (it need not exist on disk) | ||
| * @param packageJsonInfoCache (Optional) A cache for package file lookups - it's best to have a cache when this function is called often |
There was a problem hiding this comment.
| * @param packageJsonInfoCache (Optional) A cache for package file lookups - it's best to have a cache when this function is called often | |
| * @param [packageJsonInfoCache] A cache for package file lookups - it's best to have a cache when this function is called often |
(it also might make sense to put this parameter last)
| * since it switches the builtin `node` module transform. Generally speaking, if unset, | ||
| * the field is treated as though it is `ModuleKind.CommonJS`. | ||
| */ | ||
| impliedNodeFormat?: ModuleKind.ESNext | ModuleKind.CommonJS; |
There was a problem hiding this comment.
is there a nodeFormat property already? Would it be possible to call this nodeFormat? Does that name imply more certainty than this one?
There was a problem hiding this comment.
no.... nodeFormat just sounds like it might have something to do with ts.Node (since it's a property on one), while the extra word implied I'm hoping makes it more at-a-glance apparent that it has something to do with node-the-platform.
| @@ -5894,7 +5908,13 @@ namespace ts { | |||
|
|
|||
| export enum ModuleResolutionKind { | |||
| Classic = 1, | |||
| NodeJs = 2 | |||
| NodeJs = 2, | |||
| // Starting with node12, node's module resolver has significant departures from tranditional cjs resolution | |||
There was a problem hiding this comment.
Does this comment show up in the quick info of our API? If not, I think we could do with a shorter comment for future maintainers.
There was a problem hiding this comment.
No, it doesn't show up in our public API - it's more of a rationale comment than anything else.
There was a problem hiding this comment.
I just remembered—everything that looks at UsersPreferences["importModuleSpecifierEnding"] needs to completely ignore that setting and pretend it’s always "full" when --moduleResolution is node12 or nodenext. That setting is only applicable in modes where foo, foo/index, and foo/index.js are plausibly interchangeable. As it is now, I believe auto imports and string completions will default to leaving off extensions and index filenames, which won’t resolve in node-style module resolution.
|
@andrewbranch considering this doesn't add any of the new resolution rules yet (except format detection), I think that's probably fine to leave as-is until those are in place. |
|
What are we waiting on? |
|
@AlbertMarashi This is a fairly large change which we will likely land in 4.5. It likely needs further review and we will need to walk it through a few scenarios to see how well it works. |
|
Hello, your beta announcements are asking for feedback on this feature. First of all - I'm super excited about this! Great work! One thing I was really looking forward to in Node.js exports/ESM support was having example files that import from the library itself - this way we can just say Node.js supports this "self-referencing" behaviour; however TypeScript doesn't seem to understand how to import this in I've prepared a minimal reproduction here with https://github.com/benjie/typescript-self-referencing-issue TSConfig: {
"compilerOptions": {
"lib": ["es2021"],
"module": "nodenext",
"target": "es2021",
"strict": true,
"rootDir": "src",
"declaration": true,
"declarationDir": "dist",
"outDir": "dist"
},
"include": ["src"],
"exclude": ["dist"]
}TypeScript version Maybe this relates to #46212? Thanks for your hard work on TypeScript ❤️ |
|
Thanks @andrewbranch! When I did a second build afterwards it told me I was trying to overwrite source files; I tried adding exclude but that didn’t solve it. |
|
@orta on the topic of extensions, there needs to be more clarification about how to handle things during development. Specific questions:
|
|
sugar import WebStream = require('stream/web') ?? require('web-streams-polyfill/dist/ponyfill.js')try {
require('stream/web')
} catch {
require('web-streams-polyfill/dist/ponyfill.js')
} |
|
Hello, one of the features gated by Basically if a dependency has a {
"exports": {
"./subpath": {
"import": "./esm/subpath/index.js",
"require": "./commonjs/subpath/index.js",
"types": "./types/subpath/index.d.ts"
}
}
}the Would the team be open to extending this feature to reverse ref denoland/dnt#205 |
|
No. Old node (ie, pre 12), doesn't support exports. So it won't be getting support for resolution features the matching runtime doesn't have. :) As is, our checking is actually very useful for indicating if and how a package, as written, will work on the targeted module runtime. (Which, y'know, it might not) See #50153 for what we're actually thinking about doing - we know stuff like deno wants more flexible resolution, but we're not going to reduce safety for people using other platforms to achieve it. (Also, as an aside, that example export map, while vaguely functional, is incomplete - it shares types between two formats of entry points, which will create subtle (or not so subtle if the types are esm format) bugs on cross format-interop checking. Each of the |
No worries, some conflation there as it's not a Deno concern at all but just a quirk of dnt's current exports generation (for node) that made me notice this. Fair points otherwise. To pivot slightly, the default
I got that from https://devblogs.microsoft.com/typescript/announcing-typescript-4-5-beta/#packagejson-exports-imports-and-self-referencing. Is there a syntax for giving different types to each condition? |
The |
@DanielRosenwasser can we get that example in the beta post fixed for googleability? I thought we did ages ago (maybe we only fixed it in the release post) - it should be // package.json
{
"name": "my-package",
"type": "module",
"exports": {
".": {
// Entry-point for `import "my-package"` in ESM
"import":
// Entry-point for TypeScript resolution
"types": "./types-esm/index.d.ts"
// Runtime entry-point
"default": "./esm/index.js"
},
// Entry-point for `require("my-package") in CJS
"require": {
// Entry-point for TypeScript resolution
"types": "./types-cjs/index.d.cts"
// Runtime entry-point
"default": "./commonjs/index.cjs"
},
},
},
// CJS fall-back for older versions of Node.js
"main": "./commonjs/index.cjs",
// Fall-back for older versions of TypeScript (will usually be treated as cjs by older TS)
"types": "./types-esm/index.d.ts"
} |
|
Also I got to the 4.5 beta release post from https://www.typescriptlang.org/tsconfig#moduleResolution, that should be upgraded to the 4.7 release post. |
I’m really looking forward to this point, since it will allow me as a library author to simplify and deduplicate some “gotcha” workarounds. Is there an appropriate place to track this/would it be alright to file a new issue for that? |
|
I don't think we have an issue tracking it, it's just something we're keeping at the back of our minds. |
@weswigham what does that mean for community-maintained packages in DefinitelyTyped? Say we're typing a CJS library that starts shipping both module formats using ETA: it would be super helpful to point to a random package on DT that does this the "right" way already. |
Even though the TS docs and following comment say it's just a fallback :/ microsoft/TypeScript#44501 (comment)
This PR adds the basic framework for (compiler options and defaults) and core emit behavior for a new
module: node12andmodule: nodenextcompiler option. Both should be almost identical, for now.The core of this PR is allowing
.jsand.tsfiles to be interpreted as either esm or cjs format files, within the same compilation.import = requirestatements down to a helper-likecreateRequirecall, rather than error'ing and eliding them. (This gives easy access to syncrequirestatements as-needed for, eg, conditional top-level access to cjs dependencies)importcall expressions as-is, this way they can be used to pull in async (esm) dependencies within cjs-targeted files, and is otherwise normal cjs output..tsor.jsfile is determined by the enclosingpackage.jsonand itstypefield (if it is present and set tomodule,.tsand.jsfiles are esm format, otherwise they are cjs format).As far as checking goes, both options imply
esModuleInteropfor now (this only really affects default import checking and cjs module emit and may be subject to change when more checker behaviors are added later if they pickup the work this flag does). Additionally,module: node12implies amoduleResolution: node12andtarget: es2020, andmodule: nodenextimplies amoduleResolution: nodenextandtarget: esnext. The newmoduleResolutionmodes currently just redirect to the oldnoderesolution mode until they're actually implemented in another PR.Also included is a driveby fix for js declaration emit where
export {default} from "fs";in js was mistakenly serialized toexport {"fs" as default} from "fs";in a declaration file when"fs"is just a shorthand module declaration.