Skip to content

Conversation

AugustinMauroy
Copy link
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
@avivkeller
Copy link
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
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
Copy link
Member

ljharb commented Jul 25, 2025

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
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
Copy link
Member

ljharb commented Jul 26, 2025

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
Member Author

cc @ljharb synced with the pr

@ljharb
Copy link
Member

ljharb commented Jul 29, 2025

LGTM, thanks!

@AugustinMauroy AugustinMauroy requested a review from a team July 29, 2025 21:15
Copy link
Member

@JakobJingleheimer JakobJingleheimer left a comment

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?

@AugustinMauroy
Copy link
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
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
Copy link
Member Author

AugustinMauroy commented Aug 21, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: handle util.is**() depreciation
5 participants