-
Notifications
You must be signed in to change notification settings - Fork 813
Support Object.defineProperty for expando properties and CommonJS exports in JS files
#2513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| // [`prop`]: 1 | ||
| // let obj = {}; | ||
| // | ||
| // Object.defineProperty(obj, `prop`, { value: 0 }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like Strada's rename would also update the defineProperty. Not sure how possible that is now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now fixed, except initiating a rename in a string literal doesn't work, but that's a general issue for Corsa that we should fix in a separate PR.
| + y: typeof module.exports.b; | ||
| + ~~~~~~ | ||
| +!!! error TS2580: Cannot find name 'module'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`. | ||
| + }): void; | ||
| + declare const _exported_6: typeof hh; | ||
| + export { _exported_6 as "h" }; | ||
| + declare const _exported_7: () => void; | ||
| + export { _exported_7 as "i" }; | ||
| + declare const _exported_8: () => void; | ||
| + export { _exported_8 as "ii" }; | ||
| + declare const _exported_9: () => void; | ||
| + export { _exported_9 as "jj" }; | ||
| + declare const _exported_10: () => void; | ||
| + export { _exported_10 as "j" }; | ||
| + No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exports here seem to be causing module to not exist, or something? Definitely odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a declaration emit issue. The old declaration emitter wouldn't attempt to reference module.exports (it's only a CommonJS thing and can't be referenced in a .d.ts file), but instead would inline the actual type.
| + var chrome = {} | ||
| + Object.defineProperty(chrome, 'devtools', { value: {}, enumerable: true }) | ||
| + chrome.devtools.inspectedWindow = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this is consistentish, but it is weird that if I write:
var chrome = {}
chrome.devtools = {}
chrome.devtools.inspectedWindow = {}You get an error on the second line too, so expandos are sort of a way around a check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works in a JS file (but not in a TS file):
var chrome = {}
chrome.devtools = {}
chrome.devtools.inspectedWindow = {}So you could argue that using Object.defineProperty for the middle one should work too. Which is the case in Strada.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, having expando objects that are rooted in Object.defineProperty seems like a totally esoteric feature. I'm going to hold off on implementing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for Object.defineProperty in JavaScript files, specifically for:
- Expando properties (properties added to regular objects)
- CommonJS exports defined via
Object.definePropertyonexportsormodule.exports
The changes fix circularity errors and implicit-any errors by deferring type resolution, fix errors when using typeof module.exports in JSDoc, cache module symbols, prevent emission of CommonJSExport nodes in JS files, and refactor declaration emit.
Changes:
- Support for
Object.definePropertyto define CommonJS exports and expando properties in JS files - Fixes for error handling around
typeof module.exportsin JSDoc comments - Removal of erroneous errors about missing
moduleandexportsin JS files - Improved type inference for properties defined with
Object.defineProperty
Reviewed changes
Copilot reviewed 149 out of 149 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| checkExportsObjectAssignProperty.*.diff | Tests now correctly recognize exports defined via Object.defineProperty, removing false positive errors about undefined module/exports |
| assignmentToVoidZero*.diff | Fixes type handling for void 0 assignments |
| typeFromPropertyAssignment*.diff | Improved symbol and type resolution for property assignments |
| jsDeclarations*.diff | Removes spurious export var statements in emitted JS files |
| checkOtherObjectAssignProperty.*.diff | Properly recognizes properties defined via Object.defineProperty on exports |
| checkObjectDefineProperty.*.diff | Correctly infers types from Object.defineProperty calls |
| defaultPropertyAssignedClassWithPrototype.*.diff | Better type tracking for expando properties |
| nodeModulesCJSEmit*.diff | Removes incorrect export var statements in CommonJS emit |
| Various other conformance tests | Fixes related to CommonJS export handling |
|
Are you planning on putting more into this PR or is it ready? |
The PR is ready now. The tests reveal issues in declaration emit, namely:
We can address these in separate PRs. |
Co-authored-by: Copilot <[email protected]>
In this PR:
Object.defineProperty.Object.definePropertyapplied toexportsormodule.exports.typeof module.exportswhen used to define types in JSDoc comments in JS files.getModuleSymbol.Object.defineProperty.undefinedinitializers.The revised handling of CommonJS assignment declarations now ignores
undefinedinitializers when they occur as the initial assignment declaration of an export with multiple declarations. This provides for the common pattern of first initializing CommonJS exports toundefinedand then subsequently assigning the actual exported value.Fixes #2527.