GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — typings: remove unused primordials by anonrig · Pull Request #48509 · nodejs/node

Conversation

@anonrig

@anonrig anonrig commented Jun 21, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This pull request removes unused and confusing primordials such as ArrayLength which corresponds to Array.length, and even though it is not used at all it is often get confused by Array.prototype.length.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and lin72h reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@nodejs-github-bot nodejs-github-bot added the typings Issues and PRs related to internal TypeScript declarations. label Jun 21, 2023
targos
targos approved these changes Jun 21, 2023
View reviewed changes

@targos targos left a comment

Copy link
Copy Markdown
Member

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 it's ok to remove them because they are useless for the codebase, but the typings are correct. ArrayLength is a number, not a function:
CleanShot 2023-06-21 at 17 51 30@2x

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@anonrig anonrig added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Jun 21, 2023
aduh95
aduh95 previously requested changes Jun 22, 2023
View reviewed changes

@aduh95 aduh95 left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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 have a few issues with this PR:

  • Can you fix the commit title so it says unused and not invalid? There's nothing invalid about them, e.g. ArrayLength is the primordial value of Array.length, which is arguably not very useful, but certainly not invalid.
  • Should we also remove the other ones that we deem not useful? E.g. ArrayName, the primordial value of Array.name is unlikely to ever be used in the codebase. Removing only .length feels arbitrary.
  • There's a non-zero risk that someone with good intentions sees it's missing and make essentially a revert PR so the types are complete again, so could you replace them with comments explaining why they are not included?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@aduh95 aduh95 removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Jun 22, 2023
@anonrig anonrig changed the title typings: remove invalid primordials typings: remove unused/confusing primordials Jun 22, 2023
@anonrig
anonrig force-pushed the update-primordials branch from 0b4be13 to 002699b Compare June 22, 2023 15:36
@anonrig
anonrig requested a review from aduh95 June 22, 2023 15:37
@anonrig
anonrig force-pushed the update-primordials branch from 002699b to d9edc10 Compare June 22, 2023 15:38
@anonrig anonrig changed the title typings: remove unused/confusing primordials typings: remove unused primordials Jun 22, 2023
@anonrig
typings: remove unused primordials
d3bd915
@anonrig
anonrig force-pushed the update-primordials branch from d9edc10 to d3bd915 Compare June 22, 2023 15:39
@aduh95
aduh95 dismissed their stale review June 22, 2023 16:46

Still not convinced by this change, but blocking concerns have been addressed.

aduh95
aduh95 reviewed Jun 22, 2023
View reviewed changes
Comment thread typings/primordials.d.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.
@anonrig anonrig added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Jun 22, 2023
lpinca
lpinca approved these changes Jun 22, 2023
View reviewed changes
@anonrig anonrig added the commit-queue PRs queued for automated landing through the Commit Queue. label Jun 22, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jun 23, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 4d00da3 into nodejs:main Jun 23, 2023
@nodejs-github-bot

nodejs-github-bot commented Jun 23, 2023

Copy link
Copy Markdown
Collaborator

Landed in 4d00da3
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
@anonrig @RafaelGSS
typings: remove unused primordials
b03146d 
PR-URL: #48509
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2023
v20.4.0 proposal #48643
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@anonrig @Ceres6
typings: remove unused primordials
6921e48 
PR-URL: nodejs#48509
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@anonrig @Ceres6
typings: remove unused primordials
9523288 
PR-URL: nodejs#48509
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 10, 2023
@anonrig @ruyadorno
typings: remove unused primordials
1cfccb7 
PR-URL: #48509
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Sep 10, 2023
v18.18.0 proposal #49220
Merged
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
@anonrig @ruyadorno
typings: remove unused primordials
3363cfa 
PR-URL: #48509
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 left review comments
@lpinca lpinca lpinca approved these changes
+1 more reviewer
@targos targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. typings Issues and PRs related to internal TypeScript declarations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@anonrig @nodejs-github-bot @lpinca @targos @aduh95
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.