-
Notifications
You must be signed in to change notification settings - Fork 6
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
[#1238] Create a derived atom for dataset hydration from Next.js #1266
[#1238] Create a derived atom for dataset hydration from Next.js #1266
Conversation
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -32,6 +33,8 @@ const tourProviderStyles = { | |||
}; | |||
|
|||
export default function ExplorationAndAnalysisContainer() { | |||
const setExternalDatasets = useSetAtom(externalDatasetsAtom); | |||
setExternalDatasets(allExploreDatasets); |
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 current approach using setExternalDatasets
works, but it might need to be called in multiple places where we depend on the static datasets.
I was wondering if a more scalable solution would be to use our existing EnvConfigProvider
and evolve it into a more general ConfigProvider
. Like this it would centralize both our env and data configuration in one place.
The provider could be used like this (note the extra datasets prop passed):
<EnvConfigProvider
config={{
envMapboxToken: process.env.NEXT_PUBLIC_MAPBOX_TOKEN ?? '',
envApiStacEndpoint: process.env.NEXT_PUBLIC_API_STAC_ENDPOINT ?? '',
envApiRasterEndpoint: process.env.NEXT_PUBLIC_API_RASTER_ENDPOINT ?? '',
datasets: static-datasets
}}
>
{children}
</EnvConfigProvider>
And internally, the provider would handle dataset hydration:
const setExternalDatasets = useSetAtom(externalDatasetsAtom);
setExternalDatasets(allExploreDatasets);
Like that the setter won't have to be manually called several times if it's needed.
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.
How you described makes sense. My concern for now is how nicely it would work out irl considering Jotai positions itself as a replacement for context (I am reading https://jotai.org/docs/basics/comparison) - or it just means that we are really not taking full advantage of Jotai. I will put more thoughts into this from the perspective of global state management.
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.
@dzole0311 I was able to do the steps on Veda UI without errors. But, on the Next.js Veda UI, I'm getting the following when trying to access the /exploration
route:
Uncaught TypeError: right-hand side of 'in' should be an object, got undefined
setAtom react.js:141
ExplorationAnalysis exploration.tsx:38
renderWithHooks react-dom.development.js:11121
updateFunctionComponent react-dom.development.js:16290
mountLazyComponent react-dom.development.js:16765
beginWork$1 react-dom.development.js:18464
callCallback react-dom.development.js:20565
invokeGuardedCallbackImpl react-dom.development.js:20614
invokeGuardedCallback react-dom.development.js:20689
beginWork react-dom.development.js:26949
performUnitOfWork react-dom.development.js:25748
workLoopConcurrent react-dom.development.js:25734
renderRootConcurrent react-dom.development.js:25690
performConcurrentWorkOnRoot react-dom.development.js:24504
workLoop scheduler.development.js:256
flushWork scheduler.development.js:225
performWorkUntilDeadline scheduler.development.js:534
Thanks @vgeorge for testing! Did you try to rebuild and re-link the veda-ui library using this branch ( Also, to simplify testing, I’ve just published a new version of the veda-ui library with these changes and updated the dependency in the Next.js PR. So reinstalling the node packages in the Next.js PR should hopefully resolve the issue on your end. It appears to work fine in the Next.js deployment preview: https://deploy-preview-17--veda-ui-next-test.netlify.app/. Let me know if the issue persists! |
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.
Thanks for your support, @dzole0311. The issue was in my local env for the Next.js UI instance. I followed the steps and it is working as described.
Related Ticket: #1238
Related Next.js PR: developmentseed/next-veda-ui#17
Description of Changes
externalDatasetsAtom
to store datasets from host applications (eg Next.js)datasetLayersAtom
to use external datasets instead of the bundled static datasets from VEDA UINotes & Questions About Changes
{Add additonal notes and outstanding questions here related to changes in this pull request}
Validation / Testing
VEDA UI
Next.js