Skip to content

feat(util.is()): introduce - #119

Merged
brunocroh merged 17 commits into
mainfrom
feat(`util.is()`)
Oct 15, 2025
Merged

brunocroh merged 17 commits into
mainfrom
feat(`util.is()`)

Conversation

@AugustinMauroy

Copy link
Copy Markdown
Member

Description

Close #116

@AugustinMauroy
AugustinMauroy requested a review from a team July 24, 2025 15:03
@AugustinMauroy AugustinMauroy changed the title Feat(util.is()) feat(util.is()): introduce Jul 24, 2025
Comment thread recipes/util-is/README.md Outdated
Comment thread recipes/util-is/README.md Outdated
Comment thread recipes/util-is/README.md Outdated
Comment thread recipes/util-is/README.md Outdated
@avivkeller

Copy link
Copy Markdown
Member

@AugustinMauroy Can you check the original implementations and make sure these match them?

If they do, can you make that clear?

@AugustinMauroy

Copy link
Copy Markdown
Member Author

Comment thread recipes/util-is/src/workflow.ts Outdated
Comment thread recipes/util-is/src/workflow.ts Outdated
Comment thread recipes/util-is/src/workflow.ts Outdated
@AugustinMauroy

Copy link
Copy Markdown
Member Author

I'm sorry @ljharb but If you think there are better way to handle that please first update node api doc and then we will update this codemod

@ljharb

ljharb commented Jul 25, 2025

Copy link
Copy Markdown
Member

For the opinion ones, fine, but one of them is a bug and is just broken. It shouldn't matter what's in the docs, but it matters what code does.

@AugustinMauroy

Copy link
Copy Markdown
Member Author

For the opinion ones, fine, but one of them is a bug and is just broken. It shouldn't matter what's in the docs, but it matters what code does.

I'm annoyed because I'm caught between two stools. On the one hand, I agree with you. On the other, I want to follow the documentation strictly so that the org node is consistent.

@ljharb

ljharb commented Jul 26, 2025

Copy link
Copy Markdown
Member

That seems like an arbitrary and counterproductive constraint to me. The docs aren’t the source of truth, they need to reflect the actual source of truth, the code.

@AugustinMauroy

Copy link
Copy Markdown
Member Author

cc @ljharb synced with the pr

@ljharb

ljharb commented Jul 29, 2025

Copy link
Copy Markdown
Member

LGTM, thanks!

@AugustinMauroy
AugustinMauroy requested a review from a team July 29, 2025 21:15
Comment thread recipes/util-is/src/workflow.ts Outdated
Comment thread utils/src/ast-grep/import-statement.ts Outdated
Comment thread recipes/util-is/workflow.yml Outdated
Comment thread recipes/util-is/codemod.yml Outdated
Comment thread recipes/util-is/package.json Outdated

@JakobJingleheimer JakobJingleheimer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking good!

Aside from a few small things / nets, I think it's just missing support for dynamic imports?

Comment thread recipes/util-is/README.md Outdated
Comment thread recipes/util-is/README.md Outdated
Comment thread recipes/util-is/README.md Outdated
Comment thread recipes/util-is/README.md
Comment thread recipes/util-is/codemod.yml Outdated
Comment thread recipes/util-is/src/workflow.ts Outdated
Comment thread recipes/util-is/src/workflow.ts Outdated
Comment thread recipes/util-is/src/workflow.ts
Comment thread recipes/util-is/tests/expected/file-1.js
Comment thread recipes/util-is/workflow.yml Outdated
@AugustinMauroy

Copy link
Copy Markdown
Member Author

I think it's just missing support for dynamic imports?

Any of our codemod support that so if we want to support that do it in aside pr that create utility + update codemod

@JakobJingleheimer

Copy link
Copy Markdown
Member

Any of our codemod support that so if we want to support that do it in aside pr that create utility + update codemod

I think all of our migrations must support that in order to claim the deprecation is handled, which I believe is the intention. If it's best to create a shared util to do it, let's do that right away.

I thought you had previously said all of our current ones do handle this (and your message here also says they do, but I'm guessing from the rest that they actually don't?).

@AugustinMauroy

AugustinMauroy commented Aug 21, 2025 •

Copy link
Copy Markdown
Member Author

think all of our migrations must support that in order to claim the deprecation is handled, which I believe is the intention. If it's best to create a shared util to do it, let's do that right away.

I just realized that we have utility for that but I was never used

I thought you had previously said all of our current ones do handle this (and your message here also says they do, but I'm guessing from the rest that they actually don't?).

It's a misunderstanding there are any of our codemod that support that

AugustinMauroy and others added 3 commits August 21, 2025 11:12
Co-Authored-By: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com>
Comment thread recipes/util-is/README.md Outdated
Comment thread recipes/util-is/README.md Outdated
Comment thread recipes/util-is/README.md Outdated

@JakobJingleheimer JakobJingleheimer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did dynamic import support get added? I think that was my only previous request.

* 6. util.isFunction() → typeof value === 'function'
* 7. util.isNull() → value === null
* 8. util.isNullOrUndefined() → value == null
* 8. util.isNullOrUndefined() → value === null || value === undefined

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

very nit since it's a comment:

Suggested change
* 8. util.isNullOrUndefined() → value === null || value === undefined
* 8. util.isNullOrUndefined() → value == null

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, I understand :)

     null == null // true
undefined == null // true
    false == null // false
        0 == null // false
       '' == null // false

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The difference is in document.all, I believe, which doesn't apply to node but would if someone cargoculted the info into a browser.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

so what should I do ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't understand @ljharb's comment, so I'm not sure.

But this thread was based on a nit—it's not blocking. The blocking issue is supporting dynamic import.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

document.all == null but document.all !== null && typeof document.all !== 'undefined'.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah. I think we can ignore that extreme edge-case.

console.log('someValue is null');
}
if (someValue == null) {
if (someValue === null || someValue === undefined) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
if (someValue === null || someValue === undefined) {
if (someValue == null) {

@JakobJingleheimer JakobJingleheimer added blocked:upstream Depends on another PR or external change/fix missing functionality Required use-case missing and removed blocked:upstream Depends on another PR or external change/fix labels Sep 16, 2025
@AugustinMauroy

Copy link
Copy Markdown
Member Author

Dynamic import work well !
I'm not fan of differing from doc so let's not modify it

@brunocroh
brunocroh merged commit 0372fc6 into main Oct 15, 2025
30 checks passed
@JakobJingleheimer JakobJingleheimer added dep:23.x Migrate a deprecation for node 23.x and removed missing functionality Required use-case missing labels Oct 16, 2025
Comment thread recipes/util-is/README.md
| `util.isError(value)` | `Error.isError(value)` |
| `util.isFunction(value)` | `typeof value === 'function'` |
| `util.isNull(value)` | `value === null` |
| `util.isNullOrUndefined(value)` | `value === null || value === undefined` |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

need to escape || as it breaks the markdown

Image

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

How ?

@x87 x87 Nov 21, 2025 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

\|\|

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

opened a pr for that !

@JakobJingleheimer
JakobJingleheimer deleted the feat(`util.is()`) branch December 13, 2025 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dep:23.x Migrate a deprecation for node 23.x

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: handle util.is**() depreciation

6 participants