Only bind module.exports if no local definition exists #25869
Conversation
Note that this uses `lookupSymbolForNameWorker`, which is really a best-effort check since it only knows about symbols that it has already encountered. As a side-effect, even when `module` is bound as part of a `module.exports` reference, it only declares it once instead of one declaration per reference.
|
@sandersn can you also add a test with function exec() {
const module = new C(12);
return () => {
return module.exports; // should be fine because `module` is defined locally
}
}I don't think |
It is an error inside script files, but the binder sometimes creates a ModuleExports symbol because we doesn't know whether we have a commonjs module until after binding is done.
|
Nice test -- that exposed a crash that happens because we erroneously create a symbol with ModuleExports on it in a script file. Since script files, unlike module files, don't have a symbol, the checker is unable to create the synthetic type for The binder doesn't know ahead of time whether it is in a commonjs module or not -- that property is calculated lazily inside the binder, so I just added a check in the checker. |
| >12 : 12 | ||
|
|
||
| return () => { | ||
| >() => { return module.exports; } : () => any |
There was a problem hiding this comment.
This return type is wrong. AFAIK, module.exports here should be a number[].
Note that this, too, is a best-effort check since evidence of commonjs-ness may be found after a *reference* to module.exports. (A reference to module.exports alone is not enough evidence that a file is commonjs. It has to have an assignment to it.)
|
OK, I made the binder more conservative about creating a symbol for |
sandersn
deleted the
js/only-bind-module-exports-if-no-local-definition-exists
branch
Note that this uses
lookupSymbolForNameWorker, which is really abest-effort check since it only knows about symbols that it has already encountered.As a side-effect, even when
moduleis bound as part of amodule.exportsreference, it only declares it once instead of one declaration per reference.Fixes user baseline regression in webpack as found in #25866.