-
Notifications
You must be signed in to change notification settings - Fork 31
chore: bump to deck 9.1 #285
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This is a great start. I think we'll want to switch to using the MapboxOverlay since MapboxLayer has been deprecated, but it still works as-is we can do that in a follow up PR.
| import {MapRef, StaticMap, StaticMapProps} from 'react-map-gl'; | ||
| import {MapboxLayer} from '@deck.gl/mapbox/typed'; | ||
| import type {DeckProps, MapViewState} from '@deck.gl/core/typed'; | ||
| import {MapboxOverlay as MapboxLayer} from '@deck.gl/mapbox'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this actually isn't a drop-in replacement and will require some changes to work. http://deck.gl/docs/upgrade-guide#deckglmapbox
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a temporary fix I believe we can still import MapboxLayer with import MapboxLayer from '@deck.gl/mapbox/mapbox-layer'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather get this out of the way now. Im not getting any type errors, and all instances in the src/ folder implement the same props {id: ..., ...deck} where id is a string and deck is a Deck obj that may have also been changed in v9 to reflect the change. Let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API is different in MapboxOverlay. It creates a deck instance rather than getting passed an instance and that inversion would require a bit of refactoring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced the deck instance with the deck props in 03a2f0f
I see that the link shows that MapboxLayer to MapboxOverlay shows interleaved: true as the replacement. Should the updated API include this?
| import {MapRef, StaticMap, StaticMapProps} from 'react-map-gl'; | ||
| import {MapboxLayer} from '@deck.gl/mapbox/typed'; | ||
| import type {DeckProps, MapViewState} from '@deck.gl/core/typed'; | ||
| import {MapboxOverlay as MapboxLayer} from '@deck.gl/mapbox'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API is different in MapboxOverlay. It creates a deck instance rather than getting passed an instance and that inversion would require a bit of refactoring
|
Not all users (e.g. Kepler.gl) are able to upgrade to deck v9 yet, so I'm considering a hubble 1.4 release with support for the latest version of deck v8 and bumping Hubble.gl to 2.0 for deck v9 support. |
My dependent repo uses deck v9, so I'll aim this PR for 2.0. I am happy to do dig into the |
|
@chrisgervang can we get a 2.0-alpha version released from this PR? Not entirely sure how that'd happen given its monorepo configuration? I'd like to start testing with my dependent application without having to wait for this to land, especially if youre trying to ship a 1.4 version first. Thank you. |
|
@chrisgervang what your roadmap for this PR looking like? I've been waiting to see this land before getting back to the side project I've been looking to enhance with this. Anything I can help with? |
|
I'm actively updating all of the examples to use Then I'll be going through the release process of 1.4, which includes updating dependencies in the existing website and publishing a beta (and assuming all is good, immediately graduating to production). Then we can upgrade to deck 9 with this PR, and I think it could make sense to bump to 2.0 at that point. At some point soon I'd like to upgrade the website to use the latest vis.gl docusorus template instead of gatsby since it's a lot less painful to maintain. This can happen in parallel to the releases. |
|
Hey @charlieforward9 you can merge master in, I don't think they'll be many other changes |
|
@chrisgervang Ok, I'll follow your lead. Just keep me in touch and I'll help out. When you said I can merge master in, what do you mean? I do not have merge access. |
|
Oh I just meant I've pushed changes to master that you'll need to merge into your branch here |
modules/react/src/components/export-video/export-video-panel-preview.tsx
Show resolved
Hide resolved
|
Just released 1.4! If you have time, I would appreciate it if you could merge in the latest changes and test the examples in the website Then navigate to the examples in the site and try rendering different formats. I think getting 9.0 in the examples, and keeping some kind of compatibility with kepler.gl (which is on deck 8.9) are the high priority issues to release 2.0. I'm hoping to include #305, #300, and #231 in 2.0 but we don't need to consider them blockers. |
|
@chrisgervang thanks! Looking forward to it. Just an fyi, I tried an example in the website last night, specifically the Kepler one, and tried exporting some of the sample datasets as GIF, and it would not download, eventually crashing the page. Not sure if I'm doing something wrong here as I'm not too experienced with Kepler, but I wanted to point that out as I dove into this. |
|
Good to know - kepler is the most complicated example, maybe try the simplest quick start one and move up to the trips one |
modules/core/package.json
Outdated
| "build-bundle": "ocular-bundle ./bundle.ts" | ||
| }, | ||
| "dependencies": { | ||
| "@loaders.gl/core": "^3.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to use the version of loaders on master since there's an issue with GIF encoding the videos module in later versions, and we're not using anything from the newer version of loaders
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^3.4.13 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to bump to deck9.1, I need @loaders.gl/core@"^4.2.0", where is the encoding issue discussed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A fix was attempted, but it's still an issue. I have a reproduction in #305
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually still affects this branch too after removing the resolution
@chrisgervang Im seeing
I am unable to run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is close! I'm making a series of updates to prepare it for merge, and will split the features, tar/zip commits into their own PR.
For features, I may instead only go with #310, which internalizes the features prop rather than fully remove it. It does the same thing as your version but with a simpler type.
|
Ok, great to hear. Let me know if there's anything I can help with. I am eagerly waiting to implement this and advance it's capabilities with the new release. |
This reverts commit 46b66a5.
Pull Request Test Coverage Report for Build 13978428409Details
💛 - Coveralls |
|
@charlieforward9 I got ci tests working again, and the maplibre-based website examples all work 100%. The remaining issue is background color on the
|
| new MapView({ | ||
| farZMultiplier: 3, | ||
| clear: { | ||
| color: [61 / 255, 20 / 255, 76 / 255, 1] | ||
| } | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah Ive struggled to get that working, but as I integrate it into my codebase, Ill have much more time to learn and explore its internals.
Let me know when the release goes live. Thanks!
|
@charlieforward9 just released |


Tried bumping the most important (primarily those within the visgl org) dependencies up to their latest versions and fixing the build issues that came along with them.
Closes #277
Closes #283
For #309