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.

Sign up for GitHub

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

[docs] Force common hoist-non-react-statics version #13818

Merged
merged 3 commits into from
Dec 5, 2018
Merged

[docs] Force common hoist-non-react-statics version #13818

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
676b373
[docs] Force common hnrs version
eps1lon Dec 5, 2018
b30b9ee
[ci] Use node lts docker image
eps1lon Dec 5, 2018
ca47420
[ci] Use common docker image
eps1lon Dec 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
defaults: &defaults
working_directory: /tmp/material-ui
docker:
- image: circleci/node:9.10
- image: circleci/node:10.14
Copy link
Member Author

Choose a reason for hiding this comment

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

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

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 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

Copy link
Member Author

Choose a reason for hiding this comment

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

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,

# CircleCI has disabled the cache across forks for security reasons.
# Following their official statement, it was a quick solution, they
# are working on providing this feature back with appropriate security measures.
Expand Down
2 changes: 1 addition & 1 deletion docs/src/pages/demos/tables/CustomizedTable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const CustomTableCell = withStyles(theme => ({
body: {
fontSize: 14,
},
}))(props => <TableCell {...props} />);
}))(TableCell);
Copy link
Member Author

Choose a reason for hiding this comment

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

https://deploy-preview-13818--material-ui.netlify.com/demos/tables/#customized-tables looks good.

Copy link
Member

Choose a reason for hiding this comment

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

It looks good to me :)


const styles = theme => ({
root: {
Expand Down
3 changes: 3 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,9 @@
"webpack-cli": "^3.1.0",
"workbox-build": "^3.6.3"
},
"resolutions": {
"**/hoist-non-react-statics": "^3.2.1"
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to force it? I believe that the issue affects withStyles, it's on our side.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@oliviertassinari oliviertassinari Dec 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'm all in if it can make our life easier in this challenging React update API phase.

},
"sideEffects": false,
"nyc": {
"include": [
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui-styles/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
"@material-ui/utils": "^3.0.0-alpha.0",
"classnames": "^2.2.5",
"deepmerge": "^2.0.1",
"hoist-non-react-statics": "^3.0.0",
"hoist-non-react-statics": "^3.2.1",
"jss": "^9.3.3",
"jss-camel-case": "^6.0.0",
"jss-default-unit": "^8.0.2",
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
"debounce": "^1.1.0",
"deepmerge": "^2.0.1",
"dom-helpers": "^3.2.1",
"hoist-non-react-statics": "^3.0.0",
"hoist-non-react-statics": "^3.2.1",
"is-plain-object": "^2.0.4",
"jss": "^9.3.3",
"jss-camel-case": "^6.0.0",
Expand Down
13 changes: 4 additions & 9 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6893,15 +6893,10 @@ [email protected]:
resolved "https://registry.yarnpkg.com/hoek/-/hoek-4.2.1.tgz#9634502aa12c445dd5a7c5734b572bb8738aacbb"
integrity sha512-QLg82fGkfnJ/4iy1xZ81/9SIJiq1NGFUMGs6ParyjBZr6jW2Ufj/snDqTHixNlHdPNwN2RLVD0Pi3igeK9+JfA==

[email protected], hoist-non-react-statics@^2.3.1, hoist-non-react-statics@^2.5.0:
version "2.5.5"
resolved "https://registry.yarnpkg.com/hoist-non-react-statics/-/hoist-non-react-statics-2.5.5.tgz#c5903cf409c0dfd908f388e619d86b9c1174cb47"
integrity sha512-rqcy4pJo55FTTLWt+bU8ukscqHeE/e9KWvsOW2b/a3afxQZhwkQdT1rPPCJ0rYXdj4vNcasY8zHTH+jF/qStxw==

hoist-non-react-statics@^3.0.0, hoist-non-react-statics@^3.1.0:
version "3.1.0"
resolved "https://registry.yarnpkg.com/hoist-non-react-statics/-/hoist-non-react-statics-3.1.0.tgz#42414ccdfff019cd2168168be998c7b3bd5245c0"
integrity sha512-MYcYuROh7SBM69xHGqXEwQqDux34s9tz+sCnxJmN18kgWh6JFdTw/5YdZtqsOdZJXddE/wUpCzfEdDrJj8p0Iw==
[email protected], hoist-non-react-statics@^2.3.1, hoist-non-react-statics@^2.5.0, hoist-non-react-statics@^3.1.0, hoist-non-react-statics@^3.2.1:
version "3.2.1"
resolved "https://registry.yarnpkg.com/hoist-non-react-statics/-/hoist-non-react-statics-3.2.1.tgz#c09c0555c84b38a7ede6912b61efddafd6e75e1e"
integrity sha512-TFsu3TV3YLY+zFTZDrN8L2DTFanObwmBLpWvJs1qfUuEQ5bTAdFcwfx2T/bsCXfM9QHSLvjfP+nihEl0yvozxw==
dependencies:
react-is "^16.3.2"

Expand Down