Skip to content
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

Initial support for module: node12 #44501

Closed
wants to merge 8 commits into from

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Jun 8, 2021

This PR adds the basic framework for (compiler options and defaults) and core emit behavior for a new module: node12 and module: nodenext compiler option. Both should be almost identical, for now.

The core of this PR is allowing .js and .ts files to be interpreted as either esm or cjs format files, within the same compilation.

  • The esm emit branch is identical to es2020+ module emit, except we transpile TS-style import = require statements down to a helper-like createRequire call, rather than error'ing and eliding them. (This gives easy access to sync require statements as-needed for, eg, conditional top-level access to cjs dependencies)
  • The cjs emit branch modifies normal cjs emit to preserve import call 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.
  • Since we don't currently actually read in non-ts and non-js extensions (yet - that'll be a followup pull request), the mode of the .ts or .js file is determined by the enclosing package.json and its type field (if it is present and set to module, .ts and .js files are esm format, otherwise they are cjs format).

As far as checking goes, both options imply esModuleInterop for 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: node12 implies a moduleResolution: node12 and target: es2020, and module: nodenext implies a moduleResolution: nodenext and target: esnext. The new moduleResolution modes currently just redirect to the old node resolution 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 to export {"fs" as default} from "fs"; in a declaration file when "fs" is just a shorthand module declaration.

@weswigham
Copy link
Member Author

@typescript-bot pack this

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jun 8, 2021
@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 8, 2021

Heya @weswigham, I've started to run the tarball bundle task on this PR at 00d3e60. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 8, 2021

Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/104705/artifacts?artifactName=tgz&fileId=481F6DA0E8E4327055734581079A1C759EB09FE0C4041C50E8A0F65A8B9676F202&fileName=/typescript-4.4.0-insiders.20210608.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@weswigham
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 14, 2021

Heya @weswigham, I've started to run the tarball bundle task on this PR at 632b495. You can monitor the build here.

@weswigham
Copy link
Member Author

@typescript-bot test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 14, 2021

Heya @weswigham, I've started to run the extended test suite on this PR at 632b495. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 14, 2021

Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/104964/artifacts?artifactName=tgz&fileId=1B21432CD9A5F605C89C976D8B4E5AE17B6882129C5341748B3F95D68561BDA202&fileName=/typescript-4.4.0-insiders.20210614.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@weswigham
Copy link
Member Author

@typescript-bot test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 14, 2021

Heya @weswigham, I've started to run the extended test suite on this PR at 632b495. You can monitor the build here.

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple questions; the comment in esnextAnd2015.ts is the only one blocking for me.

Comment on lines +777 to +785
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();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to use forEachAncestorDirectory?

return undefined;
}
function lookupFromPackageJson(): ModuleKind.ESNext | ModuleKind.CommonJS {
const state: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. We don't have a module resolution mode that understands URLs at all, so classic mode is as close as we get.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@@ -7141,8 +7168,10 @@ namespace ts {
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the drive-by fix, right? What’s a shorthand module symbol?

Suggested change
// 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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a (member of a) declare module "foo"; defined module.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any useful comments, just a lot of typos. :)

src/compiler/types.ts Outdated Show resolved Hide resolved
* `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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a nodeFormat property already? Would it be possible to call this nodeFormat? Does that name imply more certainty than this one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it doesn't show up in our public API - it's more of a rationale comment than anything else.

src/compiler/types.ts Outdated Show resolved Hide resolved
src/compiler/transformers/module/esnextAnd2015.ts Outdated Show resolved Hide resolved
Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@weswigham
Copy link
Member Author

@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.

@andrewbranch
Copy link
Member

@renke it sounds like your watch mode issue was just filed at #46333

@benjie
Copy link

benjie commented Nov 10, 2021

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 import { foo } from "my-module" rather than import { foo } from ".." /* TODO: replace this with "my-module" */ - makes it much easier for users to copy/paste examples whilst keeping the examples co-located inside the package.

Node.js supports this "self-referencing" behaviour; however TypeScript doesn't seem to understand how to import this in module:"nodenext" mode - a could not find a declaration file for module 'my-module' error is raised.

I've prepared a minimal reproduction here with js folder working using Node.js and analogous ts folder which is not working with TypeScript (won't compile):

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 4.6.0-dev.20211110

Maybe this relates to #46212?

Thanks for your hard work on TypeScript ❤️

@andrewbranch
Copy link
Member

@benjie thanks for the feedback! It works after you do a build once and dist/index.d.ts is present. I think this is a bug—it seems like we need to map back from outDir to the root. I’ve copied your report to #46762.

@benjie
Copy link

benjie commented Nov 10, 2021

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.

@arpowers
Copy link

@orta on the topic of extensions, there needs to be more clarification about how to handle things during development. Specific questions:

  • The import/no unresolved eslint rule complains about the file not existing, recommend just disabling that rule?
  • Due to this change/standardization of convention regarding file extensions, what is a good general way of "always" handling extensions across a large TS project? (working across both browser and node)

@loynoir
Copy link

loynoir commented Dec 6, 2021

sugar import = require to require-or-createRequire and allow try-catch?

import WebStream = require('stream/web') ?? require('web-streams-polyfill/dist/ponyfill.js')
try {
  require('stream/web')
} catch {
  require('web-streams-polyfill/dist/ponyfill.js')
}

@nayeemrmn
Copy link

Hello, one of the features gated by "moduleResolution": "nodenext" is the per-export types mentioned at https://devblogs.microsoft.com/typescript/announcing-typescript-4-5-beta/#packagejson-exports-imports-and-self-referencing.

Basically if a dependency has a package.json like:

{
  "exports": {
    "./subpath": {
      "import": "./esm/subpath/index.js",
      "require": "./commonjs/subpath/index.js",
      "types": "./types/subpath/index.d.ts"
    }
  }
}

the "types" field above will not be looked up by a user of that dependency unless they have "moduleResolution": "nodenext" in their tsconfig.

Would the team be open to extending this feature to "moduleResolution": "node"? It's difficult that a library opting into the exports["subpath"].types feature extends a config requirement to its users. Sure that's the point of "moduleResolution" but I noticed that the other features gated by "moduleResolution": "nodenext" are more in the spirit of organising your own code. I think this would be very non-breaking in practice.

reverse ref denoland/dnt#205

@weswigham
Copy link
Member Author

weswigham commented Aug 6, 2022

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 require and import conditions should have it's own nested types entry. Rule of thumb: one js file, one .d.ts file. No being cute and trying to share (though you may re-export cjs format types from esm ones pretty easily). Also, order matters - conditions are matched in order, so if require or import or default or node comes before types, we'll never look at the types location, because a prior condition matched. This is export map spec'd behavior, so try not to blame us - you can't resolve compound (nested) conditions and null mappings without this behavior.)

@nayeemrmn
Copy link

nayeemrmn commented Aug 6, 2022

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.

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 "moduleResolution" value will eventually be one that supports exports, right? And if it's not breaking for TS, having the default match the latest version of Node seems logical to me since Node's resolution isn't designed to break as well.

(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 require and import conditions should have it's own nested types entry.

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?

@weswigham
Copy link
Member Author

the default "moduleResolution" value will eventually be one that supports exports, right?

The tsc --init default will probably be updated soonish, maybe. We'll probably discuss it after we handle that issue I linked. The compiler default we may change, but likely on a much longer timescale, since it's a breaking change.

@weswigham
Copy link
Member Author

weswigham commented Aug 7, 2022

I got that from

@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"
}

@nayeemrmn
Copy link

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.

@lgarron
Copy link

lgarron commented Aug 7, 2022

The compiler default we may change, but likely on a much longer timescale, since it's a breaking change.

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?

@weswigham
Copy link
Member Author

I don't think we have an issue tracking it, it's just something we're keeping at the back of our minds.

@thw0rted
Copy link

thw0rted commented Aug 8, 2022

Rule of thumb: one js file, one .d.ts file

@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 exports, but continues to avoid shipping in-package type defs. Should the corresponding @types package ship an exports field with separate .d.ts files under exports.import.types and exports.require.types?

ETA: it would be super helpful to point to a random package on DT that does this the "right" way already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.