Skip to content

[core] More aggressive transpilation #11492

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

Merged
merged 3 commits into from
May 20, 2018

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented May 19, 2018

Try @goto-bus-stop's idea out #11358

   {
     name: 'The initial cost people pay for using one component',
     webpack: true,
     path: 'packages/material-ui/build/Paper/index.js',
-    limit: '26.0 KB',
+    limit: '18.0 KB',
   },

@oliviertassinari oliviertassinari force-pushed the more-aggresive-bundling branch 2 times, most recently from 78e0921 to e291d6a Compare May 19, 2018 17:20
@TrySound
Copy link
Contributor

Update size snapshot please to see the whole picture.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented May 19, 2018

This is very encouraging, we save almost 10kB gzipped when importing a single component 😍. However, I have found a first issue on IE11. CSS precedence is all wrong. I have found the root cause 🐰

capture d ecran 2018-05-19 a 19 29 20

@oliviertassinari
Copy link
Member Author

oliviertassinari commented May 19, 2018

@TrySound Alright, I'm updating it. But I don't think that we can ask people to update it everytime they commit a change.

@oliviertassinari oliviertassinari force-pushed the more-aggresive-bundling branch from e291d6a to 6c388b0 Compare May 19, 2018 17:33
@TrySound
Copy link
Contributor

@oliviertassinari We can actually. There is matchSnapshot option which may fail in CI :)

@TrySound
Copy link
Contributor

Would you like me to send a PR with it?

@oliviertassinari
Copy link
Member Author

oliviertassinari commented May 19, 2018

@TrySound Technically, yes we can. But I mean from an operational point of view. I don't think that it worth the pain. Most of the time, we don't work on the bundling pipeline.
For instance, it's why I have moved the visual regression tests to an external service. It was a real pain in the ass to have to update all the baseline images every time we were changing a single line of CSS. An approve button streamline the experience.

@TrySound
Copy link
Contributor

What if we will just update snapshot on precommit hook?

@oliviertassinari
Copy link
Member Author

oliviertassinari commented May 19, 2018

@TrySound I have never used a single pre-commit hook. I have always found it to slow me down. Updating the snapshot takes ~30s end-to-end unless I'm missing something.

@TrySound
Copy link
Contributor

For prettier precommit hooks are quite fast. But yeah bundling on every commit will be noisy.

@TrySound
Copy link
Contributor

So let's just update snapshot manually when some changes are happened.

@@ -28,7 +28,7 @@ const generateClassName = createGenerateClassName();
// StyleSheet of the child has a higher specificity, because of the source order.
// So our solution is to render sheets them in the reverse order child->sheet, so
// that parent has a higher specificity.
let indexCounter = Number.MIN_SAFE_INTEGER;
let indexCounter = 10e10;
Copy link
Contributor

Choose a reason for hiding this comment

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

should be negative?

Copy link
Member Author

Choose a reason for hiding this comment

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

😆

@oliviertassinari oliviertassinari force-pushed the more-aggresive-bundling branch from 87bd78a to 78e3fbb Compare May 19, 2018 17:56
@oliviertassinari
Copy link
Member Author

oliviertassinari commented May 19, 2018

IE 11, Safari 10, Edge 15, Firefox 45 seems OK.
This type of change is scaring me out.

@oliviertassinari oliviertassinari force-pushed the more-aggresive-bundling branch from a8a4b0d to 6414f59 Compare May 20, 2018 09:06
@oliviertassinari oliviertassinari merged commit bacb056 into mui:master May 20, 2018
@oliviertassinari oliviertassinari deleted the more-aggresive-bundling branch May 20, 2018 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants