Closed dawidvdh closed 6 years ago
Hello @dawidvdh
Thanks for the PR. I'd like to keep this working with 15 as well. Can you make this compatible with both I can cut a regular release
Okay cool I have never had to make something cross compatible but will give this a shot!
@BerkeleyTrue I dont now if this is the correct approach but I have moved the react
and react-dom
dependancies to your devDependencies
and then added react to peerDependencies
that requires a minimum version 15.3.2
so the host can determine which react version to use, I think this creates the compatibility you were seeking?
Should redux
and redux-observable
also perhaps be moved to devDependencies
and peerDependencies
?
@dawidvdh what's the benefit of having this as a peerDep? I seem to recall npm 3 removed the need for peerdeps and that they should be avoided
@BerkeleyTrue the benefit is that this module will then use the host's version of react leaving the user to decide which version 15
or 16
to use.
Also as far as I know devDependencies
are not deprecated in npm 3, the behavior just changes so it wont automatically install the peerDependancies
but rather issue a warning when the required peerDependancies
are not found.
A few very popular libraries actually use peerDependancies
like airbnb's eslint, react-redux and even redux-observable.
the benefit is that this module will then use the host's version of react leaving the user to decide which version 15 or 16 to use.
Isn't this what npm does already. As long as you make the semvar range broad enough npm will use only one version of react.
from npm's docs on peerDeps:
In some cases, you want to express the compatibility of your package with a host tool or library, while not necessarily doing a require of this host
Our dependencies don't fit into this description. I don't see any reason to move dependencies that are required into dev-dependencies.
As far as I know given that two packages use wildly different versions of React, npm would give each of them their own copy of React.
But look this whole thread started because I wanted to use this package with react 16.rc-3
however when using the current package the following error gets logged Warning: Extra attributes from the server: data-reactid
which is due to this package still requiring and using version 15 of react.
So now I have moved these dependancies into both devDependencies
and peerDependancies
, the only reason these dependancies are in devDependencies
is for the tests to pass. But if this is incorrect, then how would you recommend I use this package with react 16.rc-3
?
Also if you have time this is a really good breakdown of peerDependancies
npm would give each of them their own copy of React.
npm doesn't know that they are wildly different copies of the same module. npm only cares about the semvar range. If the semvar range for this module is wide enough it will pick the one that is most specific for the project.
when using the current package the following error gets logged
That's because it is currently set to use 15. If you make the semvar range include both 15 and 16 then it should pick the version of react used by the rest of your project that fits within that range. npm's client is pretty smart. peerDeps have nothing to do with this.
only reason these dependancies are in devDependencies is for the tests to pass
No, this module directly requires React and React-dom. That is why they are dependencies and not devdeps.
from the article:
merge is used purely as an implementation detail
React/ReactDom is used here as an implementation detail. By the logic in the article, React/ReactDom should be deps not peerdeps. Also, I think this article was written before npm 5 which now dedupes and creates deterministic installs. If this package can require React 15 or 16 and you project requires react 16 npm will pick react 16 as a dependency for both files.
Are you suggesting that the solution should then be:
package.json
{
...
"dependencies": {
...
"react": "^15.0.0-0 || ^16.0.0-0",
"react-dom": "^15.0.0-0 || ^16.0.0-0"
...
}
...
}
Honestly I am no npm expert so I could be very wrong, the peerDependancies
to me still seem right but regardless at this point I just need this to work with react 16.
Let me know and I will change this PR.
yes that should work.
@BerkeleyTrue I have made the changes, now there is one test failing contain.js
with the following error:
Enzyme Internal Error: Enzyme expects an adapter to be configured, but found none. To
configure an adapter, you should call `Enzyme.configure({ adapter: new Adapter() })`
before using any of Enzyme's top level APIs, where `Adapter` is the adapter␊
corresponding to the library currently being tested. For example:␊
import Adapter from 'enzyme-adapter-react-15';␊
To find out more about this, see http://airbnb.io/enzyme/docs/installation/index.html
So as you will see enzyme now requires an Adapter for specific react versions, which version of the adapter should be tested on? react 15 or 16?
@dawidvdh yes that is an issue.
I think we can change strategy. Since React 16 has officially been release we can just upgrade this package to use 16 and release a major version bump. This will need changes to the test to make them pass as well.
cool @BerkeleyTrue all done, this PR should also resolve the various ones opened by greenkeeper
congrats! Release as version 1.0.0!
Upgrade various packages including react and
react-dom
to16.rc-3
.@BerkeleyTrue is there anyway we can do a
react-redux-epic: 0.4.0-rc.1
release in order start preparing to migrate over to react 16 once its stable?