-
Notifications
You must be signed in to change notification settings - Fork 35
feat(utils): cache redux extension connections to prevent duplicates #32
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
Open
PrettyCoffee
wants to merge
6
commits into
jotaijs:main
Choose a base branch
from
PrettyCoffee:fix/redux
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
783d828
chore: add husky prepare script to install automatically
PrettyCoffee a2b3cf6
refactor(utils): extract getReduxExtension
PrettyCoffee 09081dd
refactor(utils): extract createReduxConnector
PrettyCoffee f1e4355
refactor(utils): split redux connection init from subscribing
PrettyCoffee 4c0e416
feat(utils): cache redux extension connections to prevent duplicates
PrettyCoffee eef4381
refactor(utils): move redux extension tools into dedicated dir
PrettyCoffee File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import { Message } from '../types'; | ||
| import { ReduxExtension } from './getReduxExtension'; | ||
|
|
||
| type ConnectResponse = ReturnType<NonNullable<ReduxExtension>['connect']>; | ||
| export type Connector = ConnectResponse & { | ||
| // FIXME https://github.com/reduxjs/redux-devtools/issues/1097 | ||
| subscribe: (listener: (message: Message) => void) => (() => void) | undefined; | ||
| }; | ||
|
|
||
| /** Wrapper for creating connections to the redux extension | ||
| * https://github.com/reduxjs/redux-devtools/blob/main/extension/docs/API/Methods.md#connectoptions | ||
| **/ | ||
| export const createReduxConnector = ( | ||
| extension: ReduxExtension, | ||
| name: string, | ||
| ) => { | ||
| const connector = extension.connect({ name }); | ||
| return connector as Connector; | ||
| }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| export type ReduxExtension = NonNullable< | ||
| typeof window.__REDUX_DEVTOOLS_EXTENSION__ | ||
| > & { | ||
| disconnect?: () => void; | ||
| }; | ||
|
|
||
| /** Returns the global redux extension object if available | ||
| * https://github.com/reduxjs/redux-devtools/blob/main/extension/docs/API/Arguments.md | ||
| */ | ||
| export const getReduxExtension = ( | ||
| enabled = __DEV__, | ||
| ): ReduxExtension | undefined => { | ||
| if (!enabled) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const reduxExtension = window.__REDUX_DEVTOOLS_EXTENSION__; | ||
| if (!reduxExtension && __DEV__) { | ||
| console.warn('Please install/enable Redux devtools extension'); | ||
| return undefined; | ||
| } | ||
|
|
||
| return reduxExtension; | ||
| }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export { useReduxConnector } from './useReduxConnector'; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| import { useEffect, useRef } from 'react'; | ||
| import { Connector, createReduxConnector } from './createReduxConnector'; | ||
| import { ReduxExtension, getReduxExtension } from './getReduxExtension'; | ||
|
|
||
| type Connection = { | ||
| activeConnections: number; | ||
| connector: Connector; | ||
| }; | ||
| const connections: Record<string, Connection> = {}; | ||
|
|
||
| const hasActiveConnections = () => Object.keys(connections).length > 0; | ||
|
|
||
| const getConnector = (name: string, extension: ReduxExtension) => { | ||
| const existing = connections[name]; | ||
| if (existing) { | ||
| existing.activeConnections += 1; | ||
| return { connector: existing.connector, shouldInit: false }; | ||
| } | ||
|
|
||
| const connector = createReduxConnector(extension, name); | ||
| connections[name] = { activeConnections: 1, connector: connector }; | ||
| return { connector, shouldInit: true }; | ||
| }; | ||
|
|
||
| const removeConnection = (name: string, extension: ReduxExtension) => { | ||
| const existing = connections[name]; | ||
| if (!existing) return; | ||
|
|
||
| existing.activeConnections -= 1; | ||
| if (existing.activeConnections === 0) { | ||
| delete connections[name]; | ||
| } | ||
| if (!hasActiveConnections()) extension.disconnect?.(); | ||
| }; | ||
|
|
||
| interface ConnectorOptions<T> { | ||
| name: string; | ||
| enabled: boolean | undefined; | ||
| initialValue: T; | ||
| } | ||
|
|
||
| export const useReduxConnector = <T>({ | ||
| name, | ||
| enabled = __DEV__, | ||
| initialValue, | ||
| }: ConnectorOptions<T>) => { | ||
| const connectorRef = useRef<Connector>(); | ||
| const firstValue = useRef(initialValue); | ||
|
|
||
| useEffect(() => { | ||
| const extension = getReduxExtension(enabled); | ||
| if (!extension) return; | ||
|
|
||
| const cleanup = () => { | ||
| connectorRef.current = undefined; | ||
| removeConnection(name, extension); | ||
| }; | ||
|
|
||
| if (connectorRef.current) return cleanup; | ||
|
|
||
| const { connector, shouldInit } = getConnector(name, extension); | ||
| if (shouldInit) connector.init(firstValue.current); | ||
| connectorRef.current = connector; | ||
|
|
||
| return cleanup; | ||
| }, [enabled, name]); | ||
|
|
||
| return connectorRef; | ||
| }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Not sure if we used husky? (I avoid it in
jotai.)Uh oh!
There was an error while loading. Please reload this page.
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.
We use husky for devtools to enforce conventional commits but we don't need theSee comment belowpreparescript. So we can safely omit this.Uh oh!
There was an error while loading. Please reload this page.
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.
Hey, may I ask why you don't need the prepare script?
As I see it, the prepare script is required when a developer is pulling a fresh copy of the repo. It will then install the githooks you got in your
.huskydirectory into the.gitdirectory when executingnpm iand the dev would not have to executenpx husky installby themselves.And when you don't have the prepare script, a new contributor (like me) could commit with any message and no validation.
It probably doesn't / rarely matters for you as active maintainers, but I guess it could help new contributors :)
Or, if you don't want to enforce it and it should just be an optional convention, it also makes sense to remove it.
Uh oh!
There was an error while loading. Please reload this page.
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.
You're absolutely right! We do need the
preparescript. I was under the impression that husky auto-installs itself. It turns out they stopped doing that long ago.