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

[docs] Use transpiled icons directly #26268

Merged
merged 9 commits into from
May 15, 2021
Merged

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented May 12, 2021

Transpiled icons code only changes when:

  1. icons src changes
  2. build script changes
  3. babel changes

The icons source only changes if we run yarn src:icons. The other two rarely change.

It therefore makes sense to check in the transpiled code and use that directly. This saves us 2mins on a fresh docs build (my machine, probably ~4min on netlify) and hopefully some build cache.

this PR (763.41s): https://app.netlify.com/sites/material-ui/deploys/609be55eb0b6f600073ca110
#26265 (856.48s): https://app.netlify.com/sites/material-ui/deploys/609bb248818bdc0008a06cea

Granted, not automatically getting the latest build script/babel updates is dangerous. I'd hope that this unlikely causes us trouble and if it does, it'll be offset by the build perf gain.

TODO:

  • Fix macOS build cp -r seems to work on all supported dev OS

@eps1lon eps1lon added performance core Infrastructure work going on behind the scenes labels May 12, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented May 12, 2021

No bundle size changes (experimental)

Generated by 🚫 dangerJS against a4d260b

@eps1lon eps1lon force-pushed the core/faster-icons-builds branch from 8729a0a to c676f78 Compare May 12, 2021 14:46
@eps1lon eps1lon marked this pull request as ready for review May 12, 2021 14:47
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Maybe it would make sense to only keep the lib folder. It seems to be readable enough to review changes (over the src folder)

@eps1lon
Copy link
Member Author

eps1lon commented May 12, 2021

Maybe it would make sense to only keep the lib folder. It seems to be readable enough to review changes (over the src folder)

I deliberately added it. Just like I intend to add the material-icons folder. Otherwise it becomes impossible to just update the build:lib without having to download the icons.

Right now we already have the problem that you can't just update the builder script. You have to download the icons first. The icons could've changed in the meantime making it difficult to determine which changes are caused by the builder script and which are due to new icons.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 12, 2021

Just like I intend to add the material-icons folder.

If we add this folder, would it then make sense to drop the src? It seems to allow to have predictability. I'm asking as GitHub makes it hard to review PRs with many file changed. Maybe if there are fewer, it will be easier to review future changes.

@eps1lon
Copy link
Member Author

eps1lon commented May 14, 2021

If we add this folder, would it then make sense to drop the src? It seems to allow to have predictability. I'm asking as GitHub makes it hard to review PRs with many file changed. Maybe if there are fewer, it will be easier to review future changes.

Let's not bother with problems we do not have yet, ok? I can come with with a million concerns about potential future changes. But this isn't constructive right now.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 14, 2021

Let's not bother with problems we do not have yet, ok?

I don't know about you, but I could personally barely review the PR's that upgrade the icons by reading the git diff only. The GitHub's UI was really slow in the last 2 years. What has seems to be the most effective is to run a visual diff.

@eps1lon
Copy link
Member Author

eps1lon commented May 14, 2021

I don't understand what you're talking about. What did you try to review?

@oliviertassinari
Copy link
Member

@eps1lon we could snooze the discussion as it's not directly related to the problem this PR is fixing. My point was that I have felt it challenging to find the diff that is responsible for changing the generated output, which occupies >80% of the diff overall.

@eps1lon
Copy link
Member Author

eps1lon commented May 14, 2021

I really don't understand what you mean here. This PR should not have changed any icons. Just check in the transpiled output. If you found some changed icons please be explicit instead of alluding to problems vaguely. I think I deserve as much.

If you reviewed this PR by clicking on "files changed" then I would kindly suggest that you don't self-sabotage. If you know reviewing 5k+ files is not viable in the GitHub UI why continue doing it? I told you multiple times that you can review a PR by commit. Is there a reason this doesn't work for you? Maybe you're not aware what "review by commit" means?

As to reviewing 5k+ files: What is the reason you want to do this for this PR? This would help me describe the PR better. It seems like you either don't understand what this PR does or insist on reviewing transpiled files manually. In which case I'd like to kindly ask to help me understand what you think you would gain by reviewing transpiled files? Reviewing the transpiled output is certainly helpful but you should put more thought into what you gain by that and which tools help you accomplish that goal.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 14, 2021

I didn't notice any issue with the changes, all good. We have shifted with a side discussion. I propose we stop it here.

If you know reviewing 5k+ files is not viable in the GitHub UI why continue doing it?

For me, the UX of reviewing commit per commit is usually not working as well as the whole diff. The first commits are usually amended in the following ones, as the author finds issues with the initial logic. I guess it depends on how the author is handling them. It doesn't have to be so, but it's what I have usually noticed (not in your case). This is mainly why I find it easier to read the whole diff on PRs. I rarely look commit per commit, only as a last resort.

@eps1lon
Copy link
Member Author

eps1lon commented May 15, 2021

The first commits are usually amended in the following ones, as the author finds issues with the initial logic.

Sure, I don't do that though. And I would usually recommend the same to other people. But I understand that rebasing is oftentimes too complicated. Though I don't understand why you mention this here because I'm fairly certain it doesn't apply.

@eps1lon eps1lon merged commit 323b76b into mui:next May 15, 2021
@eps1lon eps1lon deleted the core/faster-icons-builds branch May 15, 2021 07:57
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 performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants