Skip to content

lib: enhance use of Map with primordials #31155

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

lib: enhance use of Map with primordials #31155

wants to merge 1 commit into from

Conversation

Sebastien-Ahkrin
Copy link
Contributor

Hello,
Now, in this PR I have added Map in the primordials eslint
so have created a line in "/lib/.eslintrc.yaml".

rules:
  no-restricted-globals:
  - name: Map
        message: "Use `const { Map } = primordials;` instead of the global."

And just import Map in some file :).

const {
  // [...]
  Map,
} = primordials;

I hope this new PR will help you :x

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 2, 2020
@Sebastien-Ahkrin
Copy link
Contributor Author

Sebastien-Ahkrin commented Jan 2, 2020 •
edited
Loading

Refs: #30697

@BridgeAR
Copy link
Member

BridgeAR commented Jan 2, 2020 •
edited
Loading

I know we also have a SafeMap. Do we still need both? Or is it enough to rely upon the primordial Map? Similar would apply to Set and their weak counterparts.

// cc @devsnek @targos

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 2, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28090/

@targos
Copy link
Member

targos commented Jan 2, 2020

SafeMap is not exactly the same, as it also provides safe calls to the prototype methods.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28135/

@Trott
Copy link
Member

Trott commented Jan 4, 2020

Commit message might be clearer as lib: use primordial for Map? That can be edited by whoever lands this, though.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28204/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28210/

@Trott
Copy link
Member

Trott commented Jan 4, 2020

Landed in 9085c03

@Trott Trott closed this Jan 4, 2020
Trott pushed a commit that referenced this pull request Jan 4, 2020
@Sebastien-Ahkrin @Trott
lib: replace Map global by the primordials
9085c03 
PR-URL: #31155
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Jan 6, 2020
@Sebastien-Ahkrin @targos
lib: replace Map global by the primordials
20ecb5d 
PR-URL: #31155
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
Merged
targos pushed a commit that referenced this pull request Jan 14, 2020
@Sebastien-Ahkrin @targos
lib: replace Map global by the primordials
0dd8605 
PR-URL: #31155
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@Sebastien-Ahkrin @BethGriggs
lib: replace Map global by the primordials
947d066 
PR-URL: #31155
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

@devnexen devnexen devnexen approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@trivikr trivikr trivikr approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@Sebastien-Ahkrin @BridgeAR @nodejs-github-bot @targos @Trott @jasnell @cjihrig @devnexen @trivikr