-
Notifications
You must be signed in to change notification settings - Fork 24
fix(globals): use globals for globals
#397
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 latest updates on your projects. Learn more about Vercel for GitHub.
|
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 refactors the hardcoded list of JavaScript global objects to use the globals library, which automatically updates with new ES specifications monthly. This change simplifies maintenance by removing the need to manually track and update global objects.
- Replaces hardcoded arrays of JavaScript globals with dynamic imports from the
globalslibrary - Moves the
globalsdependency from devDependencies to dependencies - Simplifies the primitive types mapping from an object to an array structure
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/utils/parser/constants.mjs |
Replaces manual global object lists with globals.builtin and converts primitive mapping to array |
src/utils/parser/index.mjs |
Updates primitive type transformation to work with new array-based mapping |
package.json |
Moves globals from devDependencies to dependencies |
.github/dependabot.yml |
Removes globals from linting dependency group |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #397 +/- ##
==========================================
- Coverage 74.29% 74.22% -0.08%
==========================================
Files 118 118
Lines 11025 10994 -31
Branches 695 696 +1
==========================================
- Hits 8191 8160 -31
Misses 2831 2831
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
globals\ for globalsglobals for globals
|
Blocked by nodejs/node#59421 |
|
Unrelated, but |
ovflowd
left a comment
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.
If we're going down this approach, wouldn't it be easier to use Object.keys(self)?
|
cc @aduh95 / @nodejs/collaborators |
No, firstly, see #397 (comment). Secondly, we aren't building the docs on for same version of Node.js as we are on, so the list would be incomplete |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Need to fix one thing, then this'll be good to go |
|
@avivkeller conflicts? |
01f46a8 to
53c09d8
Compare
Description
The
globalslibrary is updated with every builtin global every month (https://github.com/sindresorhus/globals/blob/main/.github/workflows/update.yml), so we can safely rely on it to populate our builtin globals.IMO this is a safer alternative than having us manually update this list.
Related Issues
Ref #319