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

Add --module preserve, support require in --moduleResolution bundler #56785

Merged
merged 32 commits into from
Jan 19, 2024

Conversation

andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Dec 14, 2023

This PR allows users targeting Bun and bundlers to write fully-typed import statements and require calls in the same file.

Background and motivation

When we first introduced --moduleResolution bundler, we asked for feedback on how often people found themselves needing to write require calls in bundler code. We heard that it was very rare, so we chose not to perform any module resolution for require in that moduleResolution mode. This simplified resolution of package.json conditional "exports" and "imports"—if we were performing module resolution at all, it must have been from an import statement (or dynamic import), therefore we could unconditionally match the import condition. This resolution strategy would only be safe and accurate if we could ensure that these imports would not be transformed into require calls before a bundler saw them—and this is exactly what ts-loader, for example, would do if it used a tsconfig.json with "module": "commonjs". To avoid this problem, we required "module": "esnext" to be used in conjunction with "moduleResolution": "bundler". This restriction also prevented usage of import x = require("dep") syntax entirely.

The motivation for lifting this restriction is that we want a better default module resolution setting for VS Code than --moduleResolution node10, which lacks support for package.json "exports" but is still the default today1. nodenext is too restrictive to be a good default, since many unconfigured projects might be targeting Bun or a bundler. We tried to switch to --moduleResolution bundler, but had to revert because JavaScript users complained that they stopped getting IntelliSense on things they required.

Adding support for require() resolution to --moduleResolution bundler solves this. Now, when module resolution is initiated from an import statement, it matches the import package.json "exports" condition, and when it’s initiated from a require call, it matches require.

--module preserve

In TypeScript files, though, require calls look like:

import x = require("dep");

which are not allowed in --module esnext. To represent the fact that Bun and most bundlers allow import and require in the same file, we need a --module mode that does the same, without assuming that imports will be transformed into require calls.

In --module preserve, ESM syntax is emitted as-written2, while TypeScript-specific CommonJS syntax (import x = require("dep") and export = {}) is allowed and emitted as its standard CommonJS equivalent output (require variable statement and module.exports assignment, respectively). (Although we expect most users to use this mode with --noEmit for use in Bun and bundlers, the way code emits, or would emit if enabled, is the basis for how TypeScript performs type checking and module resolution on imports/requires.)

--module preserve implies these defaults:

  • --moduleResolution bundler
  • --esModuleInterop
  • --resolveJsonModule

I also considered, and am open to, implying --moduleDetection force.

Footnotes

  1. Recent confusion due to bad default module resolution: Intellisense not resolving modules when using exports in package.json #56412

  2. --module preserve does not change the default behavior of import elision. Imports will never be transformed into require calls, but imports that are never used in JS-emitting positions will still be dropped, as in any other mode. To prevent unused imports from being elided, use --verbatimModuleSyntax.

@typescript-bot
Copy link
Collaborator

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

@microsoft microsoft deleted a comment from typescript-bot Dec 14, 2023
@microsoft microsoft deleted a comment from typescript-bot Dec 14, 2023
@microsoft microsoft deleted a comment from typescript-bot Dec 14, 2023
@microsoft microsoft deleted a comment from typescript-bot Dec 14, 2023
@andrewbranch
Copy link
Member Author

@typescript-bot test top200

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 14, 2023

Heya @andrewbranch, I've started to run the diff-based top-repos suite on this PR at 4142f99. You can monitor the build here.

Update: The results are in!

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

@andrewbranch Here are the results of running the top-repos suite comparing main and refs/pull/56785/merge:

Everything looks good!

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

Seems good but I am wondering about the breaking API changes; the two I noted are used by some important libraries.

tests/baselines/reference/api/typescript.d.ts Show resolved Hide resolved
impliedNodeFormat?: ResolutionMode;
},
usage: StringLiteralLike,
compilerOptions: CompilerOptions,
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I think this one is still breaking after the commit.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, the new stuff is still "breaking" at a type level, but with hidden overloads?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. The thought is that I want tool authors to notice immediately, but I don’t want it to crash when users bring a newer TS to an existing tool.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good; hopefully nobody accidentally removes that backwards compat code in a refactor but that sounds doubtful.

@andrewbranch
Copy link
Member Author

@jakebailey there’s no way to get a correct resolution mode answer in --module preserve without supplying compilerOptions. What do you think about making type-level breaking changes, but guarding against an undefined compilerOptions internally?

@jakebailey
Copy link
Member

I think making them assert with a good message would be a good idea, at the least. But, I don't know how I feel about breaking ts-node, tsimp, rollup, svelte, rush, lit-analyzer, dt-tools, etc...

If undefined, can we just assume some options, keeping those from breaking? Presumably before, we were able to answer without the compiler option, right?

@andrewbranch
Copy link
Member Author

andrewbranch commented Jan 17, 2024

Yes, the implementation will only be incorrect when --module is preserve. (By “guarding against an undefined compilerOptions” I meant “don’t throw, assume module is not preserve.”)

@jakebailey
Copy link
Member

Yes, the implementation will only be incorrect when --module is preserve. (By “guarding against an undefined compilerOptions” I meant “don’t throw, assume module is not preserve.”)

Even better!

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

Looks good to my eye, thanks for adding the back compat code.

@mrazauskas
Copy link

Perhaps this is the right place to ask. I was wondering why new ModuleKinds and ModuleResolutionKinds are getting listed only in ts.ModuleKind and ts.ModuleResolutionKind, but not in ts.server.protocol.ModuleKind and ts.server.protocol.ModuleResolutionKind enums?

For instance, "node16" or "bundler" are missing here:

export const enum ModuleResolutionKind {
Classic = "Classic",
Node = "Node",
}

Similarly "node16" is missing and this PR is not adding "preserve" here:

export const enum ModuleKind {
None = "None",
CommonJS = "CommonJS",
AMD = "AMD",
UMD = "UMD",
System = "System",
ES6 = "ES6",
ES2015 = "ES2015",
ESNext = "ESNext",
}

@andrewbranch
Copy link
Member Author

I honestly had no idea those were there. I think the only time they’d be used is for clients to send the CompilerOptionsForInferredProjects command, which today in VS Code still uses module: "CommonJS", moduleResolution: "Node", so we didn’t notice the need to add anything else. I’ll update them, thanks 👍

src/compiler/program.ts Outdated Show resolved Hide resolved
src/compiler/program.ts Outdated Show resolved Hide resolved
src/compiler/program.ts Outdated Show resolved Hide resolved
src/compiler/program.ts Show resolved Hide resolved
@typescript-bot
Copy link
Collaborator

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @mjbvz, @zkat, and @joj for you. Feel free to loop in other consumers/maintainers if necessary.

@andrewbranch
Copy link
Member Author

Thanks for the reviews @sheetalkamat and @jakebailey!

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.

8 participants