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

Add support for Redux 4.x #490

Merged
merged 6 commits into from
Jun 15, 2018
Merged

Add support for Redux 4.x #490

merged 6 commits into from
Jun 15, 2018

Conversation

ApacheEx
Copy link
Contributor

@ApacheEx ApacheEx commented Apr 19, 2018

Redux 4 is out: https://github.com/reactjs/redux/releases/tag/v4.0.0

I didn't notice any breaking changes. 💯
I have also moved some packages to peerDependencies, it allows to reduce bundle size and let user to choose which version of package use.

am I missed something?

@jacoporicare
Copy link

I assume it's important to update npm-package/package.json. At least this is the version of package.json I have in my project when I installed redux-devtools-extension. Can you please add Redux 4 there as well?

@ApacheEx
Copy link
Contributor Author

ApacheEx commented Apr 20, 2018

@jacoporicare,
thanks for info. Does it mean we need to update only ./npm-package/package.json and keep ./package.json as-is?

@jacoporicare
Copy link

Unfortunately I'm not a maintainer and I don't know what's the purpose of the package.json in the root 🙁... I just upgraded Redux to v4 in my project and noticed that peer deps are not matching so I wanted to do a PR to fix it and found yours 👍.

@ApacheEx
Copy link
Contributor Author

let's wait for maintainer decision 👌

@nickserv
Copy link

nickserv commented Apr 21, 2018

I like this change overall and using peerDependencies on the root package is a great idea, but shouldn't react-dom be a production dependency since it's used to render the extension? https://github.com/zalmoxisus/redux-devtools-extension/blob/master/src/browser/extension/options/index.js

Also according to #460 being fixed, it seems that it's safe to declare Redux 4 support.

@jacoporicare
Copy link

jacoporicare commented Apr 21, 2018

Peer doesn’t mean optional, it only gives you a choice of a version you want to use. As far as I know almost every React library/component has react and react-dom as peer dependency. The truth is that I don’t know what is the purpose of package.json in the root folder...

Edit: Now I see it’s a browser extension. Sorry, I use it only as an NPM package for easier store setup with enhancers. In that case I think there shouldn’t be any peerDependency at all as it’s a stand-alone “app”?

@nickserv
Copy link

Sorry if I was being misleading, I was referring to react-dom being in devDependencies but not peerDependencies. This means npm won't ensure react-dom is installed when this is used as a dependency of another package, which I think would break it because it's used to render the extension, not just in tests. We should add react-dom to peerDependencies or dependencies (probably the latter if it doesn't matter what version of react-dom is used by the underlying app).

@nickserv
Copy link

Cool thanks for making this consistent. I'm having second thoughts about if these need to be peer dependencies though. The redux-devtools-extension has a peer dependency on redux because it's used directly with an existing Redux app, which makes sense. However aren't the dependencies in the root package isolated to the extension (in which case they should probably not be peer dependencies)? If that's the case, it may be better to only use peer dependencies in redux-devtools-extension as we were before, which would involve reverting these changes to remotedev-redux-devtools-extension's package.json.

@ApacheEx
Copy link
Contributor Author

ApacheEx commented Apr 21, 2018

@nickmccurdy - yeah now I think we need to change only ./npm-package/package.json.

I will make change

@jacoporicare
Copy link

jacoporicare commented Apr 21, 2018

@nickmccurdy That’s exactly what I was trying to say (in the EDIT section though) 😉. Glad we are on the same page.

Copy link

@nickserv nickserv left a comment

Choose a reason for hiding this comment

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

💯I'm fine with this assuming there are no outstanding issues with Redux 4 support

@markerikson
Copy link

There does appear to be an incompatibility with Redux 4 that needs to be fixed - this line is out of date:

isHotReloaded = () => this.lastAction === '@@redux/INIT';

See reduxjs/redux#2943 for discussion.

@nickserv
Copy link

nickserv commented Apr 28, 2018

Good catch. Is it a string starting with @@redux/INIT and some random stuff appended? Should we match the start of the string or is there a better way?

@markerikson
Copy link

Feels a bit hacky, but yeah, that ought to work.

@ApacheEx
Copy link
Contributor Author

ApacheEx commented Apr 30, 2018

seems we should not import actionTypes from redux. Just match the start of the string should work as expected.

@kanzelm3
Copy link

Any ETA on when this will be merged?

@kaiwa
Copy link

kaiwa commented May 24, 2018

I just spent a reasonable amount of time figuring out why my app was broken, until I got that it is because of this issue. Please merge this.

@Dabolus
Copy link

Dabolus commented May 26, 2018

@nickmccurdy just to answer your question: yes, it is a string with @@redux/INIT and some random stuff appended. The random stuff is computed by generating a random number and doing some operations on it. The random stuff can range from not existing (when the random number is zero) to being 31 characters long. Since it is a dot separated base36 string, we are certain that it can only contain letters from a to z, numbers from 0 to 9 and dots.

To sum everything up, it is possible to use a regex such as:
/^@@redux\/INIT[a-z0-9.]{0,31}$/

Instead of using indexOf, but I'm quite sure the best option would be to keep using indexOf for multiple reasons (i.e. performance and possibly future compatibility in case the way the random string is computed changes)

@kanzelm3
Copy link

kanzelm3 commented Jun 6, 2018

Is this project still being actively maintained?

@kanzelm3
Copy link

@nickmccurdy @zalmoxisus I apologize in advance for the spam, but this is still an issue in the v2.15.2 release of the Redux DevTools extension. Considering it's been approved and CI passed, I was hoping this would have been merged weeks ago.

I really don't want to have to implement an alternative solution, because I think this extension is great. Please merge and release this fix!

@kanzelm3
Copy link

kanzelm3 commented Jun 15, 2018

To be clear, my project is using composeWithDevTools from the instructions outlined here. While I have v2.15.2 of the browser extension installed, the latest version of redux-devtools-extension available on npm is v2.13.2.

Edit: I just tried removing the redux-devtools-extension package from my project and using the window.__REDUX_DEVTOOLS_EXTENSION_COMPOSE__ approach outlined in the advanced store setup instructions, and I can confirm that the issue still exists.

@solshark
Copy link

solshark commented Jun 15, 2018 via email

@giopunt
Copy link

giopunt commented Jun 15, 2018

@nickmccurdy @zalmoxisus any update on this?

@jhen0409 jhen0409 merged commit 7db8972 into zalmoxisus:master Jun 15, 2018
@asos-giovannip
Copy link

Do we need another PR to bump the version and/or publish to npm?

@nickmccurdy @zalmoxisus

@jhen0409
Copy link
Collaborator

Published the npm package as v2.13.3. For chrome extension publish will be later.

@bbthorntz
Copy link

It seems like there are still issues when using store.replaceReducers. Redux will now dispatch a @@redux/REPLACE action rather than the previous @@redux/INIT action.

reduxjs/redux#2943

@deb0rian
Copy link

Having this during any build process with yarn:
warning " > [email protected]" has incorrect peer dependency "redux@^3.5.2".

@nickserv
Copy link

nickserv commented Aug 18, 2018

It seems like the Firefox extension hasn't been updated: https://addons.mozilla.org/en-US/firefox/addon/remotedev/

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.