-
Notifications
You must be signed in to change notification settings - Fork 2
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
Persist atom state with the veda-ui and remove jotai dependency #8
Conversation
'jotai': path.resolve(__dirname, 'node_modules', 'jotai'), | ||
'jotai-devtools': path.resolve(__dirname, 'node_modules', 'jotai-devtools'), | ||
'jotai-location': path.resolve(__dirname, 'node_modules', 'jotai-location'), | ||
'jotai-optics': path.resolve(__dirname, 'node_modules', 'jotai-optics'), |
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.
Looks like yarn/npm seem to hoist node_modules
to the top directory, if they are referenced within the project. I initially thought I'd have to write node_modules/@developmentseed/veda-ui/node_modules/jotai
, but this seems to work as well 🤔
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.
Aha,
this makes me wonder if it will be better just to export these hooks that use atoms from veda-ui side, so the client doesn't even need to install jotai separately. not a blocker, I will experiment this while resolving the modal issue.
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.
Does that mean that instances would need to install jotai as a peer dependency, if we don't ship it via the UI library? I also tried using the <Provider />
mentioned here, but couldn't get rid of the "multiple instances" problem. Might be worthwhile to re-visit again.
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.
🤔 My current thought is that instances don't even need jotai as a peer dependency, (like jotai will be a dependency of VEDA UI not the instance). I haven't tried it yet! let me try and revisit this again!
**Related Ticket:** #1158 ### Description of Changes The PR fixes `date-fns` imports which are breaking in Next.js, probably due to how Next.js handles ESM and CommonJS module imports. Next.js PR with explanation of the core issue: NASA-IMPACT/next-veda-ui#8 ### Notes & Questions About Changes _{Add additonal notes and outstanding questions here related to changes in this pull request}_ ### Validation / Testing _{Update with info on what can be manually validated in the Deploy Preview link for example "Validate style updates to selection modal do NOT affect cards"}_
Problem
This PR addresses an issue where multiple instances of Jotai were detected when using it in the Next.js instance together with the @developmentseed/veda-ui library. The main issue was that the
timelineDatasetsAtom
was not syncing between the Next.js instance and the veda-ui.This happened because both the Next.js instance and the veda-ui were importing separate instances of jotai, which created two different default stores even though they shared the same
timelineDatasetsAtom
configuration. What gave me the hint was this warning in the instance:Solution
To make sure we have a single source of truth, I added Webpack aliases in next.config.js that redirect all Jotai-related imports in the Next.js app to use the Jotai version from the veda-ui.
Screen.Recording.2024-10-29.at.14.04.58.mp4
Test