-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Conversation
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. |
@typescript-bot test top200 |
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! |
@andrewbranch Here are the results of running the top-repos suite comparing Everything looks good! |
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.
Seems good but I am wondering about the breaking API changes; the two I noted are used by some important libraries.
impliedNodeFormat?: ResolutionMode; | ||
}, | ||
usage: StringLiteralLike, | ||
compilerOptions: CompilerOptions, |
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.
Similarly here, and also used externally: https://github.com/search?q=%2F%5C.getModeForUsageLocation%2F&type=code
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 think this one is still breaking after the commit.
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.
Oh, the new stuff is still "breaking" at a type level, but with hidden overloads?
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.
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.
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.
Sounds good; hopefully nobody accidentally removes that backwards compat code in a refactor but that sounds doubtful.
@jakebailey there’s no way to get a correct resolution mode answer in |
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 If undefined, can we just assume some options, keeping those from breaking? Presumably before, we were able to answer without the compiler option, right? |
Yes, the implementation will only be incorrect when |
…se `program.getModeForResolutionAtIndex`
Even better! |
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.
Looks good to my eye, thanks for adding the back compat code.
Perhaps this is the right place to ask. I was wondering why new For instance, "node16" or "bundler" are missing here: TypeScript/src/server/protocol.ts Lines 3756 to 3759 in 55153b0
Similarly "node16" is missing and this PR is not adding "preserve" here: TypeScript/src/server/protocol.ts Lines 3745 to 3754 in 55153b0
|
I honestly had no idea those were there. I think the only time they’d be used is for clients to send the |
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. |
Thanks for the reviews @sheetalkamat and @jakebailey! |
This PR allows users targeting Bun and bundlers to write fully-typed
import
statements andrequire
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 writerequire
calls in bundler code. We heard that it was very rare, so we chose not to perform any module resolution forrequire
in thatmoduleResolution
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 theimport
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 ofimport 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 theyrequire
d.Adding support for
require()
resolution to--moduleResolution bundler
solves this. Now, when module resolution is initiated from an import statement, it matches theimport
package.json"exports"
condition, and when it’s initiated from a require call, it matchesrequire
.--module preserve
In TypeScript files, though, require calls look like:
which are not allowed in
--module esnext
. To represent the fact that Bun and most bundlers allowimport
andrequire
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")
andexport = {}
) is allowed and emitted as its standard CommonJS equivalent output (require
variable statement andmodule.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
Recent confusion due to bad default module resolution: Intellisense not resolving modules when using
exports
in package.json #56412 ↩--module preserve
does not change the default behavior of import elision. Imports will never be transformed intorequire
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
. ↩