Respect package.json "type" and module-format-specific file extensions in more module modes
#57896
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
the
For Milestone Bug
|
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. |
andrewbranch
changed the title
"type" and module-format-specific file extensions in more module modes
|
@typescript-bot test top400 |
|
@andrewbranch Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@andrewbranch Here are the results of running the top 400 repos comparing Something interesting changed - please have a look. Details
|
FWIW this does not match the behavior of esbuild or bun; both accept import statements with those file extensions: $ cat index.cts
import fs from "fs";
console.log("my code is:");
console.log(fs.readFileSync(__filename, "utf8"));
$ bun run index.cts
my code is:
import fs from "fs";
console.log("my code is:");
console.log(fs.readFileSync(__filename, "utf8"));
$ bunx esbuild --platform=node --bundle index.cts
var __create = Object.create;
var __defProp = Object.defineProperty;
var __getOwnPropDesc = Object.getOwnPropertyDescriptor;
var __getOwnPropNames = Object.getOwnPropertyNames;
var __getProtoOf = Object.getPrototypeOf;
var __hasOwnProp = Object.prototype.hasOwnProperty;
var __markAsModule = (target) => __defProp(target, "__esModule", { value: true });
var __reExport = (target, module2, copyDefault, desc) => {
if (module2 && typeof module2 === "object" || typeof module2 === "function") {
for (let key of __getOwnPropNames(module2))
if (!__hasOwnProp.call(target, key) && (copyDefault || key !== "default"))
__defProp(target, key, { get: () => module2[key], enumerable: !(desc = __getOwnPropDesc(module2, key)) || desc.enumerable });
}
return target;
};
var __toESM = (module2, isNodeMode) => {
return __reExport(__markAsModule(__defProp(module2 != null ? __create(__getProtoOf(module2)) : {}, "default", !isNodeMode && module2 && module2.__esModule ? { get: () => module2.default, enumerable: true } : { value: module2, enumerable: true })), module2);
};
// index.cts
var import_fs = __toESM(require("fs"));
console.log("my code is:");
console.log(import_fs.default.readFileSync(__filename, "utf8"));I'm not totally sure how widespread this is, though. But it does in general make me nervous about making files stricter based on |
|
@jakebailey but if you make those file extensions |
|
Works the same: $ bun run index.cjs
my code is:
import fs from "fs";
console.log("my code is:");
console.log(fs.readFileSync(__filename, "utf8"));
$ bunx esbuild --platform=node --bundle index.cjs
var __create = Object.create;
var __defProp = Object.defineProperty;
var __getOwnPropDesc = Object.getOwnPropertyDescriptor;
var __getOwnPropNames = Object.getOwnPropertyNames;
var __getProtoOf = Object.getPrototypeOf;
var __hasOwnProp = Object.prototype.hasOwnProperty;
var __markAsModule = (target) => __defProp(target, "__esModule", { value: true });
var __reExport = (target, module2, copyDefault, desc) => {
if (module2 && typeof module2 === "object" || typeof module2 === "function") {
for (let key of __getOwnPropNames(module2))
if (!__hasOwnProp.call(target, key) && (copyDefault || key !== "default"))
__defProp(target, key, { get: () => module2[key], enumerable: !(desc = __getOwnPropDesc(module2, key)) || desc.enumerable });
}
return target;
};
var __toESM = (module2, isNodeMode) => {
return __reExport(__markAsModule(__defProp(module2 != null ? __create(__getProtoOf(module2)) : {}, "default", !isNodeMode && module2 && module2.__esModule ? { get: () => module2.default, enumerable: true } : { value: module2, enumerable: true })), module2);
};
// index.cjs
var import_fs = __toESM(require("fs"));
console.log("my code is:");
console.log(import_fs.default.readFileSync(__filename, "utf8")); |
|
I think it’s ok for us to be more restrictive and opinionated than any single bundler or runtime here. It’s in Bun’s best interest to make plausibly interpretable input code execute without errors. |
|
Sure, the CJS thing isn't so bad; from my reading this PR doesn't prevent people from using the icky "set module=esnext to emit ESM" trick, so I don't think there are any extra cases I'm concerned about off the top of my head. |
weswigham
reviewed
Mar 25, 2024
| // source file. | ||
| return transformImpliedNodeFormatDependentModule; | ||
| case ModuleKind.System: | ||
| return transformSystemModule; | ||
| default: | ||
| return transformModule; |
There was a problem hiding this comment.
Should we just make this switch exhaustive? This last case is really only for ModuleKind.None, right?
jakebailey
approved these changes
Mar 26, 2024
sheetalkamat
requested changes
Apr 3, 2024
tests/baselines/reference/tsbuild/amdModulesWithOut/prepend-reports-deprecation-error.js
Show resolved
Hide resolved
sheetalkamat
approved these changes
Apr 12, 2024
andrewbranch
added a commit
that referenced
this pull request
Jun 12, 2024
…c file extensions in more module modes) (#58831)
andrewbranch
added a commit
that referenced
this pull request
Jun 13, 2024
andrewbranch
added a commit
to andrewbranch/TypeScript
that referenced
this pull request
Jun 13, 2024
… extensions in more `module` modes (microsoft#57896)" This reverts commit 585a9af.
andrewbranch
added a commit
to andrewbranch/TypeScript
that referenced
this pull request
Jun 13, 2024
… extensions in more `module` modes (microsoft#57896)" This reverts commit 585a9af.
Merged
andrewbranch
added a commit
to andrewbranch/TypeScript
that referenced
this pull request
Jun 13, 2024
… extensions in more `module` modes (microsoft#57896)" This reverts commit 585a9af.
andrewbranch
added a commit
that referenced
this pull request
Jun 13, 2024
andrewbranch
added a commit
to andrewbranch/TypeScript
that referenced
this pull request
Jun 17, 2024
…fic file extensions in more `module` modes (microsoft#57896)"" This reverts commit d3c8f08.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #54752
Fixes #50647
Closes #54788
Related #55221
Prerequisite for #54102
Suggest reviewing individual commits:
This PR allows us to use module-format-specific file extensions and package.json
"type"fields in allmodulemodes—not just innode16/nodenextas today—but the way we use them can vary bymodule/moduleResolutionmode. This has two noticeable effects:Emit respects file extension / package.json
"type"more often.moduleises2015–esnext, we will now treat.cts/.cjsfiles, as well as.ts/.jsfiles when the nearest package.json has"type": "commonjs", as CommonJS and emit CommonJS outputs accordingly. Note that unlike in--module nodenext, a lack of package.json"type"does not cause.ts/.jsfiles to be treated as CommonJS. We only override the default behavior of themodulesetting when"type"is explicitly set to"commonjs", or when the file extension is.cts/.cjs.moduleiscommonjs, we will now treat.mts/.mjsfiles, as well as.ts/.jsfiles when the nearest package.json has"type": "module", as ESM and output ESM outputs accordingly.moduleispreserve, ESM import/export declarations are now forbidden in.cts/.cjsfiles, as well as in.ts/.jsfiles when the nearest package.json has"type": "commonjs". We want to ensure that--module preservemeans the module syntax you write gets emitted without significant transformation, but we also want to prevent ESM syntax from being used in unambiguously CommonJS files, so this situation acts much likeverbatimModuleSyntaxis enabled.Unambiguously ESM-mode declaration files do not get synthesized default exports
This is ESM-only packages come with synthesized default in
moduleResolution: bundler#54752. Now that we always collect information about the package.json"type"and file extension, we gain the ability to notice when a declaration file, likely in a node_modules dependency, must represent an ESM JavaScript file, which changes how a default import targeting that file works. Previously, under allmodulemodes exceptnode16/nodenext, this was allowed:Since
foo.d.mtshas an unambiguously-ESM file extension, we should not allow a default import from a module that doesn’t declare a default export. However, it was allowed because we believed thatfoo.d.mtsmight actually represent a CommonJS module with amodule.exports.xproperty, in which case a default import should be allowed and would link to themodule.exportsobject. With this PR, when a declaration file has an ESM file extension or exists in a"type": "module"context, we use that to prevent us from letting a default import happen where it shouldn’t.This work also unlocks a fix for #54102. Webpack and esbuild use module-format-specific file extensions and package.json
"type"to vary their ESM/CJS interop rules, so if we want to provide correct types for projects using those bundlers, we will need to be able to leverage that module format info under--module esnextor--module preserve.There is one test failing that represents an pre-existing bug that was not exposed until now because the test did not previously look up package.json files, and a particular sequence of edits causes a failure in combination with that. It has been isolated to a failure that doesn’t rely on this PR’s changes in #57757 so it can be investigated separately.