Skip to content

Conversation

AugustinMauroy
Copy link
Member

Description

use modern alternative per @ljharb comment on nodejs/userland-migrations#119.

@nodejs-github-bot nodejs-github-bot added deprecations Issues and PRs related to deprecations. doc Issues and PRs related to the documentations. labels Jul 29, 2025
Co-authored-by: James M Snell <[email protected]>
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! there's a few more comments from that linked PR but this is a strict improvement, and i appreciate you getting to it before i had time to :-D

@AugustinMauroy
Copy link
Member Author

Thanks! there's a few more comments from that linked PR but this is a strict improvement, and i appreciate you getting to it before i had time to :-D

Can you confirm here that the util.is**() series is suitable for you so that we don't have to make 10 pr to correct the issue?

@ljharb
Copy link
Member

ljharb commented Jul 29, 2025

well, an alternative for isPrimitive is Object(arg) !== arg (rather than arg === null || (typeof arg !=='object' && typeof arg !== 'function')), and a proper isDate would be a try/catch around Date.prototype.toString.call, but otherwise https://github.com/nodejs/node/blob/fa06eeb08acf449c1b5ec7a243bca6b26e0e950a/doc/api/deprecations.md looks fine to me wrt util.is methods - thanks for checking.

@AugustinMauroy
Copy link
Member Author

@ljharb what do you think now ?

@targos targos added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Aug 27, 2025
@@ -1192,7 +1194,7 @@ changes:
Type: End-of-Life

The `util.isNullOrUndefined()` API has been removed. Please use
`arg === null || arg === undefined` instead.
`arg == null` instead.
Copy link
Contributor

@aduh95 aduh95 Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous suggestion was more correct, if arg is an instance of HTMLAllCollection, arg == null would be true while util.isNullOrUndefined(arg) and arg === null || arg === undefined would both be false.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

proposed here #59269 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should I revert this change ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#59269 (comment) suggests this because your original change was even more wrong (i.e. it would not treat undefined as a false negative). This new suggestion still has a blind spot (it treats document.all as a false positive), IMO yes reverting is the correct call, because this change is not an improvement.

FWIW you can find this behavior spec'd in step 4 of the IsLooselyEqual section of ECMAScript. Node.js users should encounter it only with Electron or similar.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, right. that's a fair point. in regular node code there's no such thing as document.all so it has no impact, but as a general JS guideline and in electron, @aduh95 is correct.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i had reverted change

@aduh95 aduh95 added dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 27, 2025
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 28, 2025
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 28, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/59269
✔  Done loading data for nodejs/node/pull/59269
----------------------------------- PR info ------------------------------------
Title      doc: update deprecation guide for `util.is**()` (#59269)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     AugustinMauroy:fix-utils-is -> nodejs:main
Labels     doc, deprecations, commit-queue-squash, dont-land-on-v20.x, dont-land-on-v22.x
Commits    6
 - doc: update deprecation guide for `util.is**()`
 - fix: null & undifined
 - better handling
 - Merge branch 'fix-utils-is' of https://github.com/AugustinMauroy/node…
 - fix lint
 - revert useless change
Committers 2
 - Augustin Mauroy <[email protected]>
 - GitHub <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/59269
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/59269
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 29 Jul 2025 08:03:26 GMT
   ✔  Approvals: 5
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/59269#pullrequestreview-3068530588
   ✔  - Jordan Harband (@ljharb): https://github.com/nodejs/node/pull/59269#pullrequestreview-3068945827
   ✔  - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/59269#pullrequestreview-3069084991
   ✔  - Zeyu "Alex" Yang (@himself65): https://github.com/nodejs/node/pull/59269#pullrequestreview-3069708854
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/59269#pullrequestreview-3165222795
   ✔  Last GitHub CI successful
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 59269
From https://github.com/nodejs/node
 * branch                  refs/pull/59269/merge -> FETCH_HEAD
✔  Fetched commits as b1898c541942..1175038f72a1
--------------------------------------------------------------------------------
Auto-merging doc/api/deprecations.md
[main 45209e7e6e] doc: update deprecation guide for `util.is**()`
 Author: Augustin Mauroy <[email protected]>
 Date: Tue Jul 29 10:02:40 2025 +0200
 1 file changed, 2 insertions(+), 4 deletions(-)
Auto-merging doc/api/deprecations.md
[main 9472c56a4e] fix: null & undifined
 Author: Augustin Mauroy <[email protected]>
 Date: Tue Jul 29 19:28:02 2025 +0200
 1 file changed, 1 insertion(+), 1 deletion(-)
Auto-merging doc/api/deprecations.md
error: commit fd126f502ea65a8810facf3f6d48ab30ddac6528 is a merge but no -m option was given.
fatal: cherry-pick failed
[main f230520b98] better handling
 Author: Augustin Mauroy <[email protected]>
 Date: Tue Jul 29 21:41:08 2025 +0200
 1 file changed, 4 insertions(+), 3 deletions(-)
   ✘  Failed to apply patches
https://github.com/nodejs/node/actions/runs/17299605238

@aduh95 aduh95 merged commit 42021e4 into nodejs:main Aug 28, 2025
22 checks passed
@aduh95
Copy link
Contributor

aduh95 commented Aug 28, 2025

Landed in 42021e4

@AugustinMauroy AugustinMauroy deleted the fix-utils-is branch August 28, 2025 15:00
targos pushed a commit that referenced this pull request Sep 4, 2025
PR-URL: #59269
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. deprecations Issues and PRs related to deprecations. doc Issues and PRs related to the documentations. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants