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

[core] Upgrade the dependencies #17612

Closed

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Sep 29, 2019

Exploring.

@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label Sep 29, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Sep 29, 2019

@material-ui/core: parsed: +0.50% , gzip: +0.28%
@material-ui/lab: parsed: +0.47% , gzip: +0.31%
@material-ui/system: parsed: +2.18% , gzip: +0.53%

Details of bundle changes.

Comparing: f25c643...debd185

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +0.50% 🔺 +0.28% 🔺 332,485 334,160 90,719 90,977
@material-ui/core/Paper +0.27% 🔺 +0.13% 🔺 68,882 69,071 20,527 20,553
@material-ui/core/Paper.esm +0.31% 🔺 +0.16% 🔺 62,319 62,514 19,264 19,295
@material-ui/core/Popper +0.45% 🔺 +0.50% 🔺 28,405 28,532 10,172 10,223
@material-ui/core/Textarea +0.63% 🔺 +0.33% 🔺 5,082 5,114 2,136 2,143
@material-ui/core/TrapFocus +0.42% 🔺 +0.25% 🔺 3,766 3,782 1,596 1,600
@material-ui/core/styles/createMuiTheme +0.39% 🔺 +0.19% 🔺 16,348 16,412 5,818 5,829
@material-ui/core/useMediaQuery +0.40% 🔺 +0.19% 🔺 2,488 2,498 1,050 1,052
@material-ui/lab +0.47% 🔺 +0.31% 🔺 143,352 144,027 43,792 43,926
@material-ui/styles +0.36% 🔺 +0.20% 🔺 51,641 51,826 15,353 15,383
@material-ui/system +2.18% 🔺 +0.53% 🔺 15,581 15,921 4,341 4,364
Button +0.36% 🔺 +0.19% 🔺 78,719 79,006 24,031 24,077
Modal +0.72% 🔺 +0.39% 🔺 14,542 14,646 5,113 5,133
Portal +0.62% 🔺 +0.30% 🔺 2,907 2,925 1,318 1,322
Rating +0.33% 🔺 +0.21% 🔺 70,185 70,420 21,915 21,961
Slider +0.35% 🔺 +0.18% 🔺 75,326 75,593 23,248 23,290
colorManipulator +0.89% 🔺 +0.33% 🔺 3,835 3,869 1,519 1,524
docs.landing +0.51% 🔺 +0.12% 🔺 54,267 54,542 14,344 14,361
docs.main +0.52% 🔺 +0.27% 🔺 587,126 590,154 187,881 188,379
packages/material-ui/build/umd/material-ui.production.min.js +1.00% 🔺 +1.55% 🔺 303,304 306,324 87,264 88,613

Generated by 🚫 dangerJS against debd185

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Just don't, please.

@oliviertassinari
Copy link
Member Author

Hold on, they are still 15 dependencies behind.

@eps1lon
Copy link
Member

eps1lon commented Sep 29, 2019

Hold on, they are still 15 dependencies behind.

I'm mean: Don't waste your time doing something machines can already do for us. Developer time is worth more than machine time.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Sep 29, 2019

@eps1lon I'm exploring how well the machine does the task (benchmark). It seems that dependabot would need to create significantly more pull requests to keep up with the transitive dependencies updates. Is this sustainable?

@eps1lon
Copy link
Member

eps1lon commented Sep 29, 2019

Is this sustainable?

You have to answer this question since you are proposing to do this manually. You have to outline what issue you are solving here. You have show that we currently have issues that are not solvable by doing this manually.

You are not reviewing anything here.

Edit:

I'm exploring how well the machine does the task (benchmark)

How? What exactly are you exploring here and how could you ever think this is a benchmark? You have to outline the parameters first. Unless you actually want to tell me that machine time = dev time?

@eps1lon
Copy link
Member

eps1lon commented Sep 29, 2019

You are even doing work that was already done in previous PRs. Sorry please talk to me privately first. I can't let you waste time on this.

@eps1lon eps1lon closed this Sep 29, 2019
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Sep 29, 2019

You might jump to conclusions. I can answer you once I have pushed these changes to the end and that I see how dependabot behaves in the next few weeks. I think that it's too early to tell. It might require a month.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Sep 29, 2019

My assumption is that dependabot doesn't send enough pull requests a week to keep up. I hope that by pushing the updates on this draft pull request to the end, and by looking at how much time dependabot need to keep up, we can conclude.

I have no idea what the solution would be. Maybe increase the number of allowed PRs, but it might not be enough. It feels that a few dependencies have been behind forever.

@eps1lon eps1lon closed this Sep 29, 2019
@eps1lon eps1lon reopened this Sep 29, 2019
@eps1lon
Copy link
Member

eps1lon commented Sep 29, 2019

My assumption is that dependabot doesn't send enough pull request a week to keep up.

Increase the PR count, done. Time to do stuff a machine can't do.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Sep 29, 2019

I hope it would be enough, but, will it?

  • What if we need 50 pull requests a week, not 5, and as many merge clicks / attention?
  • What if some stay stuck behind, requiring us manual intervention?

@eps1lon
Copy link
Member

eps1lon commented Sep 29, 2019

What if we need 50 pull requests a week, not 5, and as many clicks to do?

Did this happen?

What if some stay stuck behind, requiring us manual intervention?

Please elaborate with an example.

@eps1lon
Copy link
Member

eps1lon commented Sep 29, 2019

What if we need 50 pull requests a week, not 5, and as many clicks to do?

Arguing about "clicks" is not a good faith argument. Please think about what exactly you want to achieve here. What issues did you identify and how does your approach solve these.

The day "a click" is associated with a significant cost for a developer is the day nobody will every pay developers a dime anymore.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Sep 29, 2019

Aug 26, we had 30 pull requests for dependabot to keep up.

  • Each pull request has an attention overhead cost. How much? I don't know.
  • 30 PRs is a deterrent for "interested" developers that want to explore problems we are working on, (in the commits history). Not the most important dimension, but it can count. I enjoy looking at the commit history of the projects I want to learn more about.

"react-final-form": "^6.3.0",
"react-frame-component": "^4.1.1",
"react-inspector": "^3.0.2",
"react-inspector": "^4.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

What insight did you gain over #17605?

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 seems to break SSR too.

I'm not sure about using our tree view, it seems that a non-obvious layer of UI is required to reproduce Chrome output.

"nyc": "^14.1.1",
"prettier": "1.17.0",
"prettier": "1.18.2",
Copy link
Member

Choose a reason for hiding this comment

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

What insight did you gain over #16717?

@@ -123,7 +123,7 @@
"rollup-plugin-size-snapshot": "^0.10.0",
"rollup-plugin-terser": "^5.1.1",
"sinon": "^7.0.0",
"size-limit": "^0.21.0",
"size-limit": "^2.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

What insights did you gain over #16975?

@eps1lon
Copy link
Member

eps1lon commented Sep 29, 2019

Each pull request has an attention overhead cost. How much? I don't know.

Then this is not an argument. If you don't know it then you don't know it.

30 PRs is a deterrent for "interested" developers that want to explore problems we are working on, (in the commits history).

We have a changelog for that. Who is browsing commit history?

@oliviertassinari oliviertassinari force-pushed the upgrade-dependencies-v18 branch 2 times, most recently from 9515aff to c6030ee Compare September 29, 2019 15:01
@oliviertassinari
Copy link
Member Author

@eps1lon I have identified a couple of upgrades that require code changes in the codebase, that we can't automate. How would you prefer them to be handled?

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Sep 29, 2019

I stop here. They are too many dependencies to upgrade that can be handled in a single one.
It needs to be split, at least, in 3, if not more. I have found a couple of problems, some are concerning some are not:

  • gitlab has a node engine requirement on a v10, it seems to have changed between two minors. It can be mitigated by reverting, (it comes from danger)
  • next@9 is blocked by the /api limitation: Provide an option to configure the API route path vercel/next.js#8123. We might want to kill these pages my moving them in the component pages. We have explored the pros and cons. I might be on a tradeoff that solves them all. I will expose it once the concern is more important.
  • @types/styled-components: we have a compatibility issue with styled-components, it seems important to solve
  • babel-plugin-preval@3 is blocked by node features.
  • @material-ui/pickers: we have a TypeScript compatibility issue.
  • react-inspector: peer dependency warning, break SSR (window).
  • eslint@6.5.0 issue: [6.5.0] no-useless-rename with babel-eslint crash at rest properties eslint/eslint#12335
  • eslint@6.0.0 seems to break our custom eslint rules
  • Should typescript and tslint be updated?

@oliviertassinari oliviertassinari deleted the upgrade-dependencies-v18 branch February 8, 2020 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants