-
Notifications
You must be signed in to change notification settings - Fork 744
Fix hover on module.exports
#2098
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
|
The many baseline changes are all because |
| ->module.exports : Symbol(module.exports, Decl(something.js, 0, 0)) | ||
| ->module : Symbol(export=, Decl(something.js, 0, 0)) | ||
| +>module.exports : Symbol(export=, Decl(something.js, 0, 0)) | ||
| +>module : Symbol(module.exports) | ||
| +>module : Symbol("something", Decl(something.js, 0, 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.
Kind of funky that before, module.exports was module.exports and module was export=, but now module.exports is export= and module is module name...
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 think the new behavior is defensible. The old one--not so much.
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 fixes the hover display behavior for module.exports in JavaScript files and resolves a panic that occurred when clicking on exports after a module.exports = {} assignment. The fix changes how the module identifier is symbolized - previously showing Symbol(module.exports), it now displays Symbol("moduleName", Decl(...)) with the actual module name, making hover information clearer and enabling find-all-references functionality.
Key changes:
- Updated symbol representation for
moduleinmodule.exportsexpressions across all CommonJS patterns - Changed from generic
Symbol(module.exports)to specificSymbol("moduleName", Decl(...))format
|
Seems like you fixed a test and just need to run |
Tried, but didn't seem to do anything. |
|
Hmm, I'm not understanding what to do here! No changes are generated when I run |
|
I think this is a bad interaction of the script and baselines (since generating new baselines also fails the test, and the CI check is smarter) We can fix this manually for now; remove the test from the failing list, then also remove the t.Skip() call from the generated test itself, then if you run the test you can accept the baselines. Then you can rerun updatefailing again. I'll look into fixing this properly later. |
This PR fixes our odd behavior when hovering on
module.exportsin a JS file. Previously we'd display{ "exports=": { ... } }, now we displaymodule "xxx"when hovering onmoduleandexports: { ... }when hovering onexports. In addition, find-all-references onmodulefinds all locations where the module is imported.The PR also fixes the following panic: