-
-
Notifications
You must be signed in to change notification settings - Fork 660
chore: fix the NODE_ENV workaround in Provider #446
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
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pmndrs/jotai/DBQR7nFY8SJLaVW983Xt8Cn48ThL |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit b06c22c:
|
}) | ||
afterEach(() => { | ||
process.env.NODE_ENV = savedNodeEnv | ||
}) |
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.
So, does this mean onMount has some issues with DEV mode? Or, is it just a testing thing?
Needs investigation. Help wanted.
tests/async.test.tsx
Outdated
import { fireEvent, render, waitFor } from '@testing-library/react' | ||
import { Provider as ProviderOrig, atom, useAtom, Atom } from '../src/index' | ||
|
||
const Provider = process.env.PROVIDER_LESS_MODE ? Fragment : ProviderOrig | ||
const Provider = process.env.PROVIDER_LESS_MODE |
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.
Maybe extract this to a util function and use it everywhere?
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.
@dai-shi I'll do that and commit - Then you can revert if you disagree :-)
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 agree. Thanks for the suggestion.
@Thisen does a trick in #425, which seems better than my workaround.
So, this PR fixes it.
However, this PR reveals some other issues. It now shows some warnings on tests. This should actually be addressed in the second iteration of snapshot-based devtools.
Also, fixed warnings in provider-less mode tests. some of tests are missing for provider-less mode. And found an issue with atomWithQuery, which should be addressed separtely (FIXME).