Add basic CommonJS support #748
Conversation
It builds, and I think I found all the places that need to change to handle the JS versions. But I haven't hooked it up yet to find out.
The biggest tweak is very simple allowjs support in fileloader.go. Without this this PR isn't really testable.
I believe there is still an *error* but the semantics now are as if it were exported at the top level, just like Strada.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces basic CommonJS support by adding synthetic handling for CommonJS import/export patterns, including support for both module.exports assignments and property exports. Key changes include:
- Extending various transformers, checkers, and binders to recognize and properly process the new KindJSExportAssignment and KindCommonJSExport node kinds.
- Updating the parser, printer, and AST utilities to emit and consume synthetic nodes for CommonJS exports.
- Enhancing module resolution and baseline test handling to accommodate JS files and corresponding CommonJS semantics.
Reviewed Changes
Copilot reviewed 740 out of 740 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/transformers/* | Modified switch cases to include KindJSExportAssignment in various module transformation paths. |
| internal/printer/printer.go | Added new emitter functions for JSDoc types and CommonJS export declarations, and updated export assignment handling. |
| internal/parser/* | Introduced logic in the parser to create synthetic nodes for CommonJS exports and update module indicators. |
| internal/compiler/fileloader.go | Extended file resolution to correctly consider JS extensions when AllowJs is enabled. |
| internal/checker/* | Updated grammar and export assignment handling to incorporate the new JSExportAssignment kind. |
| internal/binder/* | Adjusted symbol declaration/binding logic to properly accommodate CommonJS export synthetic nodes. |
| internal/ast/* | Added new Kind values and updated AST utilities and factories to support JSExportAssignment and CommonJSExport. |
Comments suppressed due to low confidence (1)
internal/ast/kind.go:382
- [nitpick] The naming of synthetic node kinds for CommonJS exports (i.e. 'KindJSExportAssignment' vs. 'KindCommonJSExport') appears inconsistent with the open questions in the PR. Consider unifying these names for clarity and consistency across the codebase.
KindJSExportAssignment
| p.writeSpace() | ||
| p.writeKeyword("var") | ||
| p.writeSpace() | ||
| if node.Name().Kind == ast.KindStringLiteral { |
There was a problem hiding this comment.
The TODO comment in the emitCommonJSExport function notes that illegal names are not handled. Consider implementing a proper mechanism (or deferring to a follow-up issue) to generate a valid temporary name for illegal export identifiers.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
@rbuckton I'm not sure if I need a printer for commonjs exports, or any other reparsed node. If so, I'll open a bug to remind myself to emit a temp rename, or some other correct way to emit exports with arbitrary names.
This surprises me; how are we choosing to drop this?
I would not do this. It is not uncommon to depend on these working in CJS code without the node types.
I haven't looked either, but we're missing most of the code that makes
I would definitely be eliminating diffs. |
| @@ -1392,22 +1393,6 @@ func IsFunctionPropertyAssignment(node *Node) bool { | |||
| return false | |||
| } | |||
There was a problem hiding this comment.
we have a private and public duplicate of this function so I deleted the private one
| @@ -1473,10 +1458,6 @@ func IsEffectiveExternalModule(node *SourceFile, compilerOptions *core.CompilerO | |||
| return IsExternalModule(node) || (isCommonJSContainingModuleKind(compilerOptions.GetEmitModuleKind()) && node.CommonJSModuleIndicator != nil) | |||
| } | |||
| @@ -1646,20 +1627,7 @@ func IsEnumConst(node *Node) bool { | |||
| } | |||
|
|
|||
| func ExportAssignmentIsAlias(node *Node) bool { | |||
There was a problem hiding this comment.
I believe this was for syntactic module.exports= support and will never be used.
| @@ -378,12 +380,16 @@ func GetSymbolNameForPrivateIdentifier(containingClassSymbol *ast.Symbol, descri | |||
| } | |||
|
|
|||
| func (b *Binder) declareModuleMember(node *ast.Node, symbolFlags ast.SymbolFlags, symbolExcludes ast.SymbolFlags) *ast.Symbol { | |||
| container := b.container | |||
There was a problem hiding this comment.
commonjs exports are file exports, even if they're nested inside something else (though Corsa, not Strada, gives a diagnostic too)
| if !ast.IsAmbientModule(node) && (hasExportModifier || b.container.Flags&ast.NodeFlagsExportContext != 0) { | ||
| if !ast.IsLocalsContainer(b.container) || (ast.HasSyntacticModifier(node, ast.ModifierFlagsDefault) && b.getDeclarationName(node) == ast.InternalSymbolNameMissing) { | ||
| return b.declareSymbol(ast.GetExports(b.container.Symbol()), b.container.Symbol(), node, symbolFlags, symbolExcludes) | ||
| if !ast.IsAmbientModule(node) && (hasExportModifier || container.Flags&ast.NodeFlagsExportContext != 0) { |
There was a problem hiding this comment.
commonjs exports need to bind module.exports.x locally not just x, so I skipped it for now. See Open Questions
| @@ -922,6 +936,19 @@ func (b *Binder) bindFunctionExpression(node *ast.Node) { | |||
| b.bindAnonymousDeclaration(node, ast.SymbolFlagsFunction, bindingName) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
this is a port, with some advanced module code not done
| @@ -12,6 +12,7 @@ type NameResolver struct { | |||
| Error func(location *ast.Node, message *diagnostics.Message, args ...any) *ast.Diagnostic | |||
| Globals ast.SymbolTable | |||
| ArgumentsSymbol *ast.Symbol | |||
| RequireSymbol *ast.Symbol | |||
There was a problem hiding this comment.
this allows error-free use of require even without @types/node installed. This is here as part of the port of const x = require code from Strada. I'm torn about including it; module.exports= does error if you don't have node types installed, and I think that's better. Thoughts? I'll make a note to remove it for now.
internal/checker/checker.go
Outdated
| @@ -13547,15 +13553,6 @@ func (c *Checker) resolveSymbolEx(symbol *ast.Symbol, dontResolveAlias bool) *as | |||
|
|
|||
| func (c *Checker) getTargetOfImportEqualsDeclaration(node *ast.Node, dontResolveAlias bool) *ast.Symbol { | |||
| // Node is ImportEqualsDeclaration | VariableDeclaration | |||
There was a problem hiding this comment.
const a = require('x').a is no longer supported
| @@ -16980,16 +17023,13 @@ func (c *Checker) getTypeOfAlias(symbol *ast.Symbol) *Type { | |||
| } | |||
| targetSymbol := c.resolveAlias(symbol) | |||
| exportSymbol := c.getTargetOfAliasDeclaration(c.getDeclarationOfAliasSymbol(symbol), true /*dontRecursivelyResolve*/) | |||
| declaredType := c.getExportAssignmentType(exportSymbol) | |||
| // It only makes sense to get the type of a value symbol. If the result of resolving | |||
| // the alias is not a value, then it has no type. To get the type associated with a | |||
| // type symbol, call getDeclaredTypeOfSymbol. | |||
| // This check is important because without it, a call to getTypeOfSymbol could end | |||
| // up recursively calling getTypeOfAlias, causing a stack overflow. | |||
| if links.resolvedType == nil { | |||
There was a problem hiding this comment.
this was only needed to support old syntactic module.exports=
| /// module.exports.name = expr | ||
| jsDeclarationKindExportsProperty | ||
| /// className.prototype.name = expr | ||
| jsDeclarationKindPrototypeProperty |
There was a problem hiding this comment.
I included the last 3 entries for future use, although I'm not 100% sure that they will be needed, or stay private to this module.
| + | ||
| + module.exports = /** @type {FooFun} */(void 0); | ||
| + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
| +!!! error TS2309: An export assignment cannot be used in a module with other exported elements. |
There was a problem hiding this comment.
typedefs are implicitly exported, and need to not be in the presence of module.exports=. I'm going to fix this separately because it's going to be tricky.
There was a problem hiding this comment.
Thinking about this more, I'll probably need to bring back delayedBindJSDocTypedef from Strada. Unfortunate, but it will still be simpler than Strada's, I promise.
| import test from "./test"; | ||
| + ~~~~~~~~ | ||
| +!!! error TS2306: File 'test.js' is not a module. | ||
| export type test | ||
| - |
There was a problem hiding this comment.
I suspect it's a difference in error reporting ("Type expected" and "'=' expected" appear to be on the same location), but I'll confirm.
There was a problem hiding this comment.
Yep. The code is working as expected in Corsa but not in Strada. The two messages are both on the EOF token in Corsa. I'm not sure why they evade position-based deduping in Strada.
| +==== app.js (2 errors) ==== | ||
| + function require(a) { | ||
| + ~~~~~~~ | ||
| +!!! error TS2441: Duplicate identifier 'require'. Compiler reserves name 'require' in top level scope of a module. |
There was a problem hiding this comment.
Hm, why is this happening? Maybe a module mode related problem?
There was a problem hiding this comment.
I'm not sure why the dupe is happening, but the checker is supposed to avoid resolving require to the global one if there is a local one available, so I need to stop that from happening.
In other words, the const fs = require('fs') below is not supposed to be an import, just a call to the local function.
| +=== /jsx.jsx === | ||
| + |
There was a problem hiding this comment.
Odd baseline, maybe a bug in the printer?
|
Some replies:
The only examples I found in top200 JS were webpack test cases.
Even if you have
I moved this up to the type tag PR and will consider re-adding non-duplicate reparsed nodes after Corsa is done and we don't care about baseline diffs anymore. |
|
Haven’t looked at the code yet, but from the description, I will note that exports.a = "a";is more common than module.exports.a = "a";in my experience. (Our own CommonJS emit, for example, does this.) (The way to reason about this, if it helps: the module system will use the value of
When searching for this, make sure you're looking for the
I would not do this—it seems like we’re making CommonJS much less powerful, so I’m not sure it makes sense to make it more nitpicky at the same time. I think the broad questions about what can be dropped depends on the high level goal/philosophy here. If I were writing new CommonJS code (??), I would be happy with the level of support you have here. But existing code does all kinds of weird stuff, and I don’t think you’re going to find much of it in the |
Thanks, my intuitions about that are out-of-date.
The customers I'm trying to support are: people writing TS-in-JSDoc, people writing miscellaneous unchecked JS, and people browsing emitted JS in node_modules. The only code base in topNNN that I've seen with commonjs is webpack, and it uses only a single form ( |
This contingency will likely be harmed by the removal of any require/exports-related analysis, unfortunately. |
testdata/baselines/reference/submoduleAccepted/compiler/modulePreserve4.errors.txt.diff
Outdated
Show resolved
Hide resolved
This removes support for `module.exports=` and `module.exports.x=` in the same file. As part of this, it also removes support for implicitly exported typedefs, which I plan to add back via support in the binder.
Plus several other bug fixes. I still need to look over the baselines to figure out remaining bugs and limitations.
|
The latest commit resolves I still need to look through the baselines for other bugs and limitations, and fix up the errors that you all have already pointed out. |
| +>module.exports : any | ||
| +>module : any | ||
| +>exports : any | ||
| +>module.exports : (foo: typeof Foo) => string |
There was a problem hiding this comment.
typeof Foo is correct -- the typeof-insertion insertion shenanigans in Strada were doing the wrong thing.
| @@ -45,9 +35,5 @@ | |||
| +!!! error TS1110: Type expected. | |||
| +==== types3.ts (1 errors) ==== | |||
There was a problem hiding this comment.
weird difference in spacing between Strada and Corsa baselines.
| { | ||
| // 'exports' does not provide a contextual type to a function-class | ||
| exports.Cls = function() { | ||
| ->exports.Cls = function() { this.x = 0; } : typeof Cls | ||
| ->exports.Cls : typeof Cls | ||
| ->exports : typeof import("/a") | ||
| ->Cls : typeof Cls | ||
| ->function() { this.x = 0; } : typeof Cls | ||
| +>exports.Cls = function() { this.x = 0; } : () => void | ||
| +>exports.Cls : any |
There was a problem hiding this comment.
I'm not sure why exports.Cls : any but it's not a problem with exports resolution, because exports.x works above. I suspect it's something wrong with partially implemented construction function typing.
|
Is there anything left in this PR? It's hard to view and the thread is long so I'm not sure where we are at; I tried to skim the code until the tab crashed, and I'm not sure there's anything that jumps out at me, especially if this is a net improvement over the current state that we can update later. |
This BETTER not re-add those extra baselines!
|
@jakebailey You're probably right, I've just been fixing bugs that would be easier to review individually. Still,
|
This PR adds basic CommonJS imports and exports. It's aiming to cover 80% of usage--a subset of TS semantics in most cases.
CommonJS imports are implemented basically the same way as in Strada. The binder and checker treat a variable declaration that matches a syntactic pattern as if it were the same as
import x = require('x')in Typescript.const x = require('x')is supported.const { a, b, c } = require('x')is also supported (this is different than TS, but works the same as in Strada).const a = require('x').ais no longer supported.CommonJS export assignments are implemented as a synthetic variant of Typescript export assignments. When the parser sees
module.exports = ..., it emits a synthetic JSexport = ....module.exports = xis supported, with the same semantics asexport = xin TS. (This means that literal types don't widen becauseexport=isn't a mutable location.)CommonJS exports are implemented as a separate synthetic declaration. I don't create synthetic variable declarations because
export var a = 1both exportsaand declares a locala.module.exports.a = 1only does the former. More on that in Open Questions.module.exports.a = 1exportsa, and allows references tomodule.exports.ain that file.Other changes:
const x = requiresupport (2) synthetic export support. But I haven't completely cleaned house, especially in the binder.reparser.go, and will move the JSDoc tag code there later.Open questions
Should we support local declaration of
module.exports.x=? It is possible to hack the checker and name resolution to make it possible to refer tomodule.exports.xinside the CommonJS file, but it won't be pretty. I'm leaning toward not supporting this, at least until I can run top400 JS on Corsa after this PR. Based on the top100 JS corpus I looked at earlier, it's rare to reference an exported object within the exporting file.A secondary question is: should
module,exportsandrequireerror without a declaration, suggesting that you install@types/node? Strada never does this for JS, but in Corsa I think it makes sense to ask people withcheckJson to install@types/nodeif they're using CommonJS. This does introduce lots of errors into the baselines, but remember that plain JS files report only syntax errors.How much of the ES/CJS interop failures in modulePreserve2 and 4, and importNonExportedMember12 are due to mistakes/omissions in this PR and how much are due to unfinished module exports code? I need somebody who understands ES/CJS interop well to look at these baselines.
Naming: The synthetic node for
module.exports=isKindJSExportAssignment, based onexport='sKindExportAssignment. Formodule.exports.x=, I choseKindCommonJSExport, but maybe it should beKindCommonJSExportAssignmentorKindCommonJSExportVariableDeclaration. Opinions?Should baselines skip all reparsed nodes? Just non-duplicate reparsed nodes? Skipping all of them makes the Strada baseline diffs smaller but omits, especially for
@typedef, which was not baselined before.Future work
module.exports['illegal name']=-- requires generating a temp name to print it in a .d.ts file.Requires exporting
ImplicitlyExportedwith the rest of the exportsa,b,c. The most straightforward way is to synthesise a temp name for{a,b,c}and a namespace with the same name that merges with it. The synthetic namespace contains the implicitly exported type.