chore: Refactor deck.gl plugins to Typescript #24933
Conversation
|
/testenv up |
|
@kgabryje Container image not yet published for this PR. Please try again when build is complete. |
|
@kgabryje Ephemeral environment creation failed. Please check the Actions logs for details. |
Codecov Report
@@ Coverage Diff @@
## master #24933 +/- ##
==========================================
+ Coverage 69.00% 69.17% +0.17%
==========================================
Files 1906 1900 -6
Lines 74133 73931 -202
Branches 8208 8224 +16
==========================================
- Hits 51153 51144 -9
+ Misses 20857 20663 -194
- Partials 2123 2124 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
/testenv up |
|
@kgabryje Ephemeral environment spinning up at http://54.201.186.155:8080. Credentials are |
| @@ -31,8 +31,7 @@ | |||
| "d3-array": "^1.2.4", | |||
| "d3-color": "^1.4.1", | |||
| "d3-scale": "^3.0.0", | |||
| "deck.gl": "8.5.2", | |||
| "jquery": "^3.4.1", | |||
| @@ -73,7 +73,7 @@ export interface Dataset { | |||
| main_dttm_col: string; | |||
| // eg. ['["ds", true]', 'ds [asc]'] | |||
| order_by_choices?: [string, string][] | null; | |||
| time_grain_sqla?: string; | |||
| time_grain_sqla?: [string, string][]; | |||
There was a problem hiding this comment.
I think this change is incorrect. If you search for time_grain_sqla in the codebase you'll see it's a string in many places outside the DeckGL plugin.
There was a problem hiding this comment.
I think that in the context of Dataset it actually should be an array. It doesn't really make sense for a dataset to have a single value for time grain, it should rather be a list of supported time grains. Also, if you look at Redux, that's what time_grain_sqla is.
There was a problem hiding this comment.
Oh, I think you're right! I was thinking about form_data but that makes sense in the context of the Dataset. That's the problem with bad naming.
There was a problem hiding this comment.
Thank you for the PR @kgabryje.
I think there's a problem with the time_grain_sqla definition.
| bottomMargin: 0, | ||
|
|
||
| export type DeckGLContainerState = { | ||
| lastUpdate?: number | null; |
There was a problem hiding this comment.
Can optional be sufficient here or null has a different meaning than undefined?
There was a problem hiding this comment.
It could be undefined because it wasn't defined in the initial state, and only later it was set to a number or null.
It was confusing so now lastUpdate is initialised with null and the type is lastUpdate: number | null;. Thanks for catching that!
kgabryje
force-pushed
the
chore/deckgl-typescript
branch
from
5d3ba71
to
1c96206
Compare
|
Ephemeral environment shutdown and build artifacts deleted. |
mistercrunch
added
🏷️ bot
SUMMARY
This PR refactors all javascript files under
legacy-preset-chart-deckgldirectory to Typescript.deck.gllibrary is bumped from 8.5.2 to 8.8.27 as 8.8 is the first version that exposes type declarations.The goal is to make deck.gl plugins easier to maintain and catch up with the code quality standards of the newer plugins
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No visual changes
TESTING INSTRUCTIONS
All deck.gl plugins should work as before.
ADDITIONAL INFORMATION