Replace defaultValue with ??

[jjspace]

The builtin JS Nullish coalescing operator ?? behaves exactly the same as the defaultValue function

Transition options

1. All at once - replace all instances of it's use in one big PR
   • There is a way to restrict usage of the function in eslint to help everyone transition
2. Stop using the function and prefer using ?? for all new code
3. Continue using defaultValue to match existing code
   • If this, do we specifically recommend against/restrict the use of ???

Considerations

• This is part of the public API (defaultValue), how many people use it downstream?
• What should we do with defaultValue.EMPTY_OBJECT? (Related defaultValue.EMPTY_OBJECT missing from documentation and Typescript #11326)
  • Is there a performance implication with replacing it with {} everywhere?
  • If we keep it as a constant where should it be defined?

[ggetz]

Overall, the more we can automate things with eslint rules, the better. If we do chose to replace it all at once, we should have any eslint configurations in one commit, and the actual replacement in its own commit to make merging as easy as possible.

This is part of the public API (defaultValue), how many people use it downstream?

I agree we'd want to take stock of the usage before removing it. If we do chose to remove it, we'd go through a deprecation period where we transition users off.

Is there a performance implication with replacing it with {} everywhere

Yes. This creates a new object every time, whereas defaultValue.EMPTY_OBJECT uses one, frozen object. There is a similar explanation in the defaultValue section of the Coding Guide.

[dave-b-b]

@ggetz Not sure whether this is still needed, but I can do this. I'm going to drop the other one I volunteered for.

[dave-b-b]

On this, should I replace all references to defaultValue.EMPTY_OBJECT with {}
There are some places in the code where it now reads: <some value> ?? [].

To keep things uniform, should I replace the previously hardcoded [] with defaultValue.EMPTY_ARRAY and create an export in the defaultValue.js?

[ggetz]

Thanks @dave-b-b!

@jjspace Can you please advise on this?

[jjspace]

On this, should I replace all references to defaultValue.EMPTY_OBJECT with {}

We should replace this with another global value, maybe an export called defaultEmptyObject or something? As @ggetz said earlier, if we replace all of them with {} that "creates a new object every time, whereas defaultValue.EMPTY_OBJECT uses one, frozen object. There is a similar explanation in the defaultValue section of the Coding Guide."

We may want to do it with the empty array as well but I don't know if we use that as often as the empty object. Start with just the object and make note of how many [] there are in the PR and we can discuss further.

I know it'll probably just be a very large tedious PR but feel free to do it in pieces and open it whenever you're ready. If you want to do it in chunks and open a draft PR i'm happy to take a look early just to confirm you're on the right track or anything.

[dave-b-b]

@jjspace Thanks for the feedback. Let me do a PR right now.

And it looks like there are two references to [] and one reference to ""

[dave-b-b]

@jjspace Sorry for not creating the pull request. The tests are failing.

I'm trying to figure out how to run individual tests in the debugger so that I can trace the issue, but am having troubling figuring out how to get it set it. Here's the type of error I'm getting:

[jjspace]

I'm trying to figure out how to run individual tests in the debugger so that I can trace the issue, but am having troubling figuring out how to get it set it.

You can run single tests or suites by putting "f" in front of the it or describe block like fit or fdescribe. Check out our Testing Guide for more info.

In the specific test you showed it looks like a value that was previously guaranteed to be an array (or something else) using defaultValue may no longer be there.

[dave-b-b]

@jjspace Thanks for the testing guide. I fixed it. (I accidentally deleted a () so it was passing a function instead of an array. But I fixed that and created a partial pull request.

[dave-b-b]

@jjspace Just realized there are 2500 of these references. I think I'll break this in half and submit two halves. Will probably be a little more digestible this way.

[jjspace]

Just realized there are 2500 of these references

@dave-b-b 😅 that's why I said it would probably be a very large and tedious but simple PR. I think it'd be best if we have 1 final commit that we can merge and then possibly tag before and after to make it easy to merge into other PRs like I'll have to do for my prettier PR.

If you still want to split it up we can use the first PR as a sort of "staging" branch and you can create one or more smaller PRs into that one to review smaller chunks, like this:

gitGraph
    commit
    commit
    branch first-half
    checkout first-half
    commit
    branch second-half
    commit
    checkout first-half
    merge second-half
    checkout main
    merge first-half
    commit

Loading