Replace defaultValue with ??
#12207
Replace defaultValue with ??
#12207
Conversation
This is a partial commit to check whether I am making the correct changes
- accidentally changed createDefaultTerrainProviderViewModels() to createDefaultTerrainProviderViewModels
|
Thank you for the pull request, @dave-b-b! ✅ We can confirm we have a CLA on file for you. |
Just FYI, as long as you don't specifically use the exact keywords that Github set up it won't automatically link a PR to an Issue. |
| @@ -303,7 +302,7 @@ Object.defineProperties(BillboardGraphics.prototype, { | |||
| * @type {Property|undefined} | |||
| */ | |||
| pixelOffsetScaleByDistance: createPropertyDescriptor( | |||
| "pixelOffsetScaleByDistance" | |||
| "pixelOffsetScaleByDistance", | |||
There was a problem hiding this comment.
It seems like you have the wrong version of prettier installed or your editor configuration is wrong. These changes are distracting from the intended change of this PR (I'm working on the update in #12206). Can you do a npm install and make sure to run npm run prettier? Also probably worth checking that the pre-commit hook is working as intended so these can't even be committed.
There was a problem hiding this comment.
Okay. I see the pre-commit hook. There was probably something I did wrong. But it looks like it is preventing commits now.
I ran prettier and merged the changes into my pull request.
| @@ -2265,7 +2256,7 @@ function updateZoomTarget(viewer) { | |||
|
|
|||
| const scene = viewer.scene; | |||
| const camera = scene.camera; | |||
| const zoomOptions = defaultValue(viewer._zoomOptions, {}); | |||
| const zoomOptions = viewer._zoomOptions ?? defaultValue.EMPTY_OBJECT; | |||
There was a problem hiding this comment.
The defaultValue.EMPTY_OBJECT is frozen and cannot be modified. Most of the places we use it are only reading information from the object so this is not a problem. However some places we used defaultValue we do modify the properties later (like this one). There's probably discussion to be had about whether these objects should be modified for later but for the sake of keeping this PR isolated let's just preserve the existing behavior.
Basically don't replace calls like these with the EMPTY_OBJECT, just keep the {}. I think this is what's causing your test failures.
There was a problem hiding this comment.
@jjspace I'll keep working on this so we can merge everything at once. Thanks for working with me! And sorry for the late response.
|
@dave-b-b any progress on the rest of the replacements? Also please follow these instructions to merge in recent prettier changes |
|
Hi @dave-b-b, thank you for the work on this so far! There's a related PR open which could be dependent on this one. Would you mind letting us know your expected timeline for working on this PR? We just want to figure out what makes the most sense logistically. 😃 |
|
@ggetz Yes, sorry for the delay. I saw the related PR. I will have this done no later than the end of the week. But I'm going to try to get this done today. |
|
Closing in favor of #12252 |
Description
This is a partial pull request to verify that I am making the right kinds of changes.
Issue number and link
This is a partial fix. I don't want to link here and have this automatically close the ticket. But the ticket is #11674.
Testing plan
I ran
npm run test. It failed some of the tests, but none of the tests related to the changes that I made. (It seemed like they were all timeout issues)Author checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change