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

refactor(utils): extract redux extension logic into wrapper functions #38

Merged
merged 2 commits into from
Mar 13, 2023

Conversation

PrettyCoffee
Copy link
Contributor

Chunk of this PR: #32

This will create a redux devtools extension wrapper to make it easier to use and prevent the repetition of the code.
Furthermore adds some documentation and links for better DX.

It should not change any functionality but only do some refactoring.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 11, 2023

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 3dcdf45:

Sandbox Source
React Configuration
React Typescript Configuration

@PrettyCoffee PrettyCoffee changed the title refactor(utils): extract getReduxExtension refactor(utils): extract redux extension logic into wrapper functions Mar 11, 2023
@arjunvegda arjunvegda requested a review from dai-shi March 11, 2023 14:59
@dai-shi
Copy link
Member

dai-shi commented Mar 13, 2023

Thanks for working on this.

Just to confirm and be on the same page, I hoped #33 would fix it, but extention.disconnect() closing all connections prevents it, correct?
I wonder if this is an issue with Redux tools, and if we report it, there's possibility that they can fix (so that #33 works).

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.

Other than that, the refactoring looks okay.

Comment on lines 1 to 2
export * from './getReduxExtension';
export * from './createReduxConnection';
Copy link
Member

Choose a reason for hiding this comment

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

This is just a preference of mine, but I'd avoid index stuff.
Can you simply import them directly?

@PrettyCoffee
Copy link
Contributor Author

PrettyCoffee commented Mar 13, 2023

Thanks for working on this.

I am happy to help :)

Just to confirm and be on the same page, I hoped #33 would fix it, but extention.disconnect() closing all connections prevents it, correct?

Yes, I personally think you shouldn't merge #33. When unrendering a single useAtomDevtools it will disconnect all of them and not only the one where it is used.

I wonder if this is an issue with Redux tools, and if we report it, there's possibility that they can fix (so that #33 works).

Actually no, that's not bug, it is the expected behavior.

The extension itself owns all connections:

image

When calling extension.disconnect(), all connections should be disconnected.

The only thing we could do is creating a feature request for disconnecting single connections, like connection.disconnect() or extension.disconnect(connection) if that makes sense.
Not sure what their thoughts are on that since redux uses the extension completely differently.

I can create a issue for that and reference it 👍

Edit: Created an issue and mentioned it in #32

@PrettyCoffee PrettyCoffee force-pushed the refactor/redux-devtools branch from 33c53e0 to 3dcdf45 Compare March 13, 2023 17:30
@dai-shi
Copy link
Member

dai-shi commented Mar 13, 2023

The only thing we could do is creating a feature request for disconnecting single connections, like connection.disconnect() or extension.disconnect(connection) if that makes sense.

It makes sense. I get the point.

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.

As long as this is just refactoring without changing logic, it should be safe to merge.

Copy link
Member

@arjunvegda arjunvegda left a comment

Choose a reason for hiding this comment

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

Thanks a ton again for your work!

@arjunvegda arjunvegda merged commit 2fca2e8 into jotaijs:main Mar 13, 2023
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.

3 participants