Skip to content
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

feat(devtools): enabled option useAtomsDevtools #1076

Merged
merged 22 commits into from
Apr 15, 2022

Conversation

Aslemammad
Copy link
Member

This PR tries to achieve what we achieved in pmndrs/zustand#880 so we can add an option called enabled for better testing/dev env friendly errors/warnings!

This PR is for useAtomsDevtools, so we can make reviewing easier. after this, we'll try achieving this for other dev tools.

@vercel
Copy link

vercel bot commented Mar 30, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pmndrs/jotai/CVgMg3mQqHUk2BTzcvpM6TZYs7HJ
✅ Preview: https://jotai-git-fork-aslemammad-fix-enabled-useatomsdevtools-pmndrs.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 30, 2022

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 a17d4bc:

Sandbox Source
React Configuration
React Typescript Configuration
React Browserify Configuration
React Snowpack Configuration
Next.js Configuration
Next.js with custom Babel config Configuration
React with custom Babel config Configuration

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first review

src/devtools/useAtomsDevtools.ts Outdated Show resolved Hide resolved
src/devtools/useAtomsDevtools.ts Outdated Show resolved Hide resolved
src/devtools/useAtomsDevtools.ts Outdated Show resolved Hide resolved
src/devtools/useAtomsDevtools.ts Outdated Show resolved Hide resolved
src/devtools/useAtomsDevtools.ts Outdated Show resolved Hide resolved
Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

second one!

src/devtools/useAtomsDevtools.ts Show resolved Hide resolved
@@ -117,18 +136,19 @@ export function useAtomsDevtools(name: string, scope?: Scope) {
let extension: typeof window['__REDUX_DEVTOOLS_EXTENSION__']

try {
extension = window.__REDUX_DEVTOOLS_EXTENSION__
extension =
((enabled ?? __DEV__) && window.__REDUX_DEVTOOLS_EXTENSION__) || undefined
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add | false in L136 instead.

Suggested change
((enabled ?? __DEV__) && window.__REDUX_DEVTOOLS_EXTENSION__) || undefined
((enabled ?? __DEV__) && window.__REDUX_DEVTOOLS_EXTENSION__)

console.warn('Please install/enable Redux devtools extension')
}
}

if (!store[DEV_SUBSCRIBE_STATE]) {
if (!store[DEV_SUBSCRIBE_STATE] && enabled && __DEV__) {
throw new Error('useAtomsSnapshot can only be used in dev mode.')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this completely, right?

const ScopeContext = getScopeContext(scope)
const { s: store, w: versionedWrite } = useContext(ScopeContext)

if (!store[DEV_SUBSCRIBE_STATE]) {
if (!store[DEV_SUBSCRIBE_STATE] && enabled && __DEV__) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still want the early return.
Which test fails? I mean it should only fail if enabled === false, right? It should be a new test.

Suggested change
if (!store[DEV_SUBSCRIBE_STATE] && enabled && __DEV__) {
if (enabled === false) {
return
}
if (!store[DEV_SUBSCRIBE_STATE]) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely forgot about this being a hook. We can't early return. That's really my bad. So sorry to have your time wasted. Added a commit: dc485b7

@dai-shi dai-shi added this to the v1.6.4 milestone Apr 14, 2022
@dai-shi dai-shi changed the title fix: enabled option useAtomsDevtools feat(devtools): enabled option useAtomsDevtools Apr 14, 2022
@dai-shi dai-shi merged commit a4b5c30 into pmndrs:main Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants