Uh oh!
There was an error while loading. Please reload this page.
Respect package.json "type" and module-format-specific file extensions in more module modes - #57896
Merged
Andrew Branch (andrewbranch) merged 16 commits intoApr 12, 2024
Conversation
Andrew Branch (andrewbranch)
requested a review
from Sheetal Nandi (sheetalkamat)March 21, 2024 22:28
TypeScript Bot (typescript-bot)
commented
Mar 21, 2024
Contributor
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 Daniel Rosenwasser (@DanielRosenwasser) and Ryan Cavanaugh (@RyanCavanaugh) are aware of the changes, just as a heads up. |
Andrew Branch (andrewbranch)
requested review from
Daniel Rosenwasser (DanielRosenwasser) and Wesley Wigham (weswigham)March 21, 2024 22:28
"type" and module-format-specific file extensions in more module modesAndrew Branch (andrewbranch)
commented
Mar 21, 2024
MemberAuthor
TypeScript Bot (@typescript-bot) test top400 |
Contributor
TypeScript Bot (typescript-bot)
commented
Mar 21, 2024
Contributor
Andrew Branch (@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: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TypeScript Bot (typescript-bot)
commented
Mar 22, 2024
Contributor
Andrew Branch (@andrewbranch) Here are the results of running the top 400 repos comparing Something interesting changed - please have a look. Details
|
Jake Bailey (jakebailey)
commented
Mar 22, 2024
Member
FWIW this does not match the behavior of esbuild or bun; both accept import statements with those file extensions: $ cat index.ctsimport fs from "fs";console.log("my code is:");console.log(fs.readFileSync(__filename, "utf8"));
$ bun run index.ctsmy code is:import fs from "fs";console.log("my code is:");console.log(fs.readFileSync(__filename, "utf8"));
$ bunx esbuild --platform=node --bundle index.ctsvar __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.ctsvar 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 |
Andrew Branch (andrewbranch)
commented
Mar 22, 2024
MemberAuthor
Jake Bailey (@jakebailey) but if you make those file extensions |
Jake Bailey (jakebailey)
commented
Mar 22, 2024
Member
Works the same: $ bun run index.cjsmy code is:import fs from "fs";console.log("my code is:");console.log(fs.readFileSync(__filename, "utf8"));
$ bunx esbuild --platform=node --bundle index.cjsvar __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.cjsvar import_fs = __toESM(require("fs"));console.log("my code is:");console.log(import_fs.default.readFileSync(__filename, "utf8")); |
Andrew Branch (andrewbranch)
commented
Mar 25, 2024
MemberAuthor
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. |
Jake Bailey (jakebailey)
commented
Mar 25, 2024
Member
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. |
| 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?
MemberAuthor
There was a problem hiding this comment.
AMD/UMD too
Uh oh!
There was an error while loading. Please reload this page.
Jake Bailey (jakebailey)
approved these changes
Mar 26, 2024
Jake Bailey (jakebailey)
left a comment
Member
There was a problem hiding this comment.
With #57931 I think this is all good.
Sheetal Nandi (sheetalkamat)
requested changes
Apr 3, 2024
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Andrew Branch (andrewbranch)
requested a review
from Sheetal Nandi (sheetalkamat)April 8, 2024 17:45
Sheetal Nandi (sheetalkamat)
approved these changes
Apr 12, 2024
Andrew Branch (andrewbranch) added a commit
that referenced
this pull request
Jun 12, 2024
…c file extensions in more module modes) (#58831)
Andrew Branch (andrewbranch) added a commit
that referenced
this pull request
Jun 13, 2024
Andrew Branch (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.
Andrew Branch (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
Andrew Branch (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.
Andrew Branch (andrewbranch) added a commit
that referenced
this pull request
Jun 13, 2024
Andrew Branch (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.
This file contains hidden or 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
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
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.