Repository navigation
Create synthetic exports symbol for commonjs module - #42655
Nathan Shively-Sanders (sandersn) merged 3 commits into
Conversation
Previously, the `module` identifier in commonjs modules got a synthetic type with a single property `exports`. The exports property reused the file's symbol, which, for a module file, gives the correct exported properties. However, the name of this symbol was still the filename of the file, not `exports`. This PR creates a synthetic symbol for `exports` by copying in a similar way to esModuleInterop's `default` symbol in `resolveESModuleSymbol` (although the intent there is to strip off signatures from the symbol).
TypeScript Bot (typescript-bot)
added
the
For Milestone Bug
Nathan Shively-Sanders (sandersn)
changed the title
exports symbol for commonjs module
| const fileSymbol = getSymbolOfNode(getSourceFileOfNode(symbol.valueDeclaration)); | ||
| const result = createSymbol(fileSymbol.flags, "exports" as __String); | ||
| result.declarations = fileSymbol.declarations ? fileSymbol.declarations.slice() : []; | ||
| result.parent = fileSymbol.parent; |
There was a problem hiding this comment.
The parent should be reset to the module symbol in module.exports, no?
| if (fileSymbol.members) result.members = new Map(fileSymbol.members); | ||
| if (fileSymbol.exports) result.exports = new Map(fileSymbol.exports); |
There was a problem hiding this comment.
How much of a memory hit for big checkJs projects (that use module.exports) is this?
There was a problem hiding this comment.
The only baseline we have for this is "does chrome-devtools-frontend run out of memory?". I'll run the user tests.
A priori, it shouldn't be gigantic; a file with 1000 exports will add 1000 map entries. The first page of google only returns an SO answer from a V8 developer, which points out that each entry has key,value and next pointers. So...is that 6000 bytes per large file? It's not too expensive, but on the other hand this is a mostly cosmetic feature. The exception is the bug that the PR fixes.
|
TypeScript Bot (@typescript-bot) user test this |
|
Heya Nathan Shively-Sanders (@sandersn), I've started to run the parallelized community code test suite on this PR at 5289808. You can monitor the build here. |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
User test baselines finished OK, so I think this is safe enough. |
Previously, the
moduleidentifier in commonjs modules got a synthetic type with a single propertyexports. The exports property reused the file's symbol, which, for a module file, gives the correct exported properties.However, the name of this symbol was still the filename of the file, not
exports. This PR creates a synthetic symbol forexportsby copying in a similar way to esModuleInterop'sdefaultsymbol inresolveESModuleSymbol(although the intent there is to strip off signatures from the symbol).It's not possible to use
cloneSymbolbecause that function merges the symbols, altering the original to point to the new symbol.Fixes #41764