[docs] Force common hoist-non-react-statics version #13818
Conversation
eps1lon
added
docs
eps1lon
force-pushed
the
docs/one-resolution-to-rule-them-all
branch
from
4162d71 to
676b373
Compare
|
Tested it locally with |
| @@ -1,7 +1,7 @@ | |||
| defaults: &defaults | |||
| working_directory: /tmp/material-ui | |||
| docker: | |||
| - image: circleci/node:9.10 | |||
| - image: circleci/node:10.14 | |||
There was a problem hiding this comment.
node 9 has reached its end of life anyway. So this change should be made regardless. Node 10 is in Active LTS state. See https://github.com/nodejs/Release#release-schedule
There was a problem hiding this comment.
This is good to me. Can we update all the occurrences?
I have added the Node.js LTS support version breaking change in the list for v4.0.0
https://material-ui.com/getting-started/supported-platforms/#server
There was a problem hiding this comment.
Right there were multiple ones.
But this has nothing to do with our dependencies. It might only be interesting for local development but we're not documenting a required node version or hinting at one via nvm,
| @@ -16,7 +16,7 @@ const CustomTableCell = withStyles(theme => ({ | |||
| body: { | |||
| fontSize: 14, | |||
| }, | |||
| }))(props => <TableCell {...props} />); | |||
| }))(TableCell); | |||
There was a problem hiding this comment.
| @@ -172,6 +172,9 @@ | |||
| "webpack-cli": "^3.1.0", | |||
| "workbox-build": "^3.6.3" | |||
| }, | |||
| "resolutions": { | |||
| "**/hoist-non-react-statics": "^3.2.1" | |||
There was a problem hiding this comment.
Do we need to force it? I believe that the issue affects withStyles, it's on our side.
There was a problem hiding this comment.
It's just safer this way. The major bump was never needed in the first place so all this essentially does is backporting all 3.x changes to 2.x.
There was a problem hiding this comment.
I find the major bump very interesting. It's ensuring people won't have a broken version.
For the resolutions part, I have one concern with the fact it might hide some problem when having multiple version installed.
There was a problem hiding this comment.
That's what I wanted to address. It forces other packages to also pull in 3.x. There was no actual breaking change between 2.x and 3.x. It "just" patches the unmaintained packages. Another analogy is that the version of hnrs version 2.x || 3.x is equivalent to the semantic version ^2.0.0. It would have the same effect on the node_modules layout. The yarn docs describe the use case in more detail.
The issue of multiple versions is not specific to hnrs. If a package uses a broken dependency then it should be fixed. Either by patching upstream or using a patched fork (or other non-conventional methods like patch-package).
I also want to emphasize that this (intentionally) does not affect users. How they configure their build is their concern. This is only for our workspace.
There was a problem hiding this comment.
I'm all in if it can make our life easier in this challenging React update API phase.
| @@ -1,7 +1,7 @@ | |||
| defaults: &defaults | |||
| working_directory: /tmp/material-ui | |||
| docker: | |||
| - image: circleci/node:9.10 | |||
| - image: circleci/node:10.14 | |||
There was a problem hiding this comment.
This is good to me. Can we update all the occurrences?
I have added the Node.js LTS support version breaking change in the list for v4.0.0
https://material-ui.com/getting-started/supported-platforms/#server
| @@ -16,7 +16,7 @@ const CustomTableCell = withStyles(theme => ({ | |||
| body: { | |||
| fontSize: 14, | |||
| }, | |||
| }))(props => <TableCell {...props} />); | |||
| }))(TableCell); | |||
TL;DR:
[email protected]removed the integrity checksums. The new image uses the latest yarn versionForce a single version of
hoist-non-react-staticsby using theresolutionsfield andyarn. This should get rid of all issues concerning hoisting statics andforwardRef.I also bumped the version of hnrs in our
package.jsonso that the fixes are propagated faster. I think if somebody upgrades our version he doesn't necessarily upgrade transitive dependencies.One caveat with
resolutionsis that it seems to be ignored if I add a package to a workspace. Runningyarnwill fix that.Closes #13776