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] Remove babel-node for server/shared modules #15764

Merged
merged 8 commits into from
May 25, 2019
Merged

[core] Remove babel-node for server/shared modules #15764

merged 8 commits into from
May 25, 2019

Conversation

cvanem
Copy link
Contributor

@cvanem cvanem commented May 20, 2019

#15497 (comment)

Replaces script usage of babel-node with node. With the exception of:

docs:api - This links into material-ui package code.
docs:i18n - Errors without babel-node. Looks like this relies on BABEL_ENV=test to resolve module paths.

@cvanem cvanem changed the title Remove babel-node [core] Remove babel-node May 20, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented May 20, 2019

Details of bundle changes.

Comparing: f14ab42...6e55b5d

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core 0.00% 0.00% 315,971 315,971 86,587 86,587
@material-ui/core/Paper 0.00% 0.00% 67,870 67,870 20,158 20,158
@material-ui/core/Paper.esm 0.00% 0.00% 61,152 61,152 18,947 18,947
@material-ui/core/Popper 0.00% 0.00% 28,740 28,740 10,352 10,352
@material-ui/core/Textarea 0.00% 0.00% 5,513 5,513 2,378 2,378
@material-ui/core/TrapFocus 0.00% 0.00% 3,744 3,744 1,580 1,580
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 15,960 15,960 5,782 5,782
@material-ui/core/useMediaQuery 0.00% 0.00% 2,106 2,106 975 975
@material-ui/lab 0.00% 0.00% 139,101 139,101 42,772 42,772
@material-ui/styles 0.00% 0.00% 51,353 51,353 15,176 15,176
@material-ui/system 0.00% 0.00% 14,458 14,458 4,184 4,184
Button 0.00% 0.00% 84,061 84,061 25,582 25,582
Modal 0.00% 0.00% 20,344 20,344 6,684 6,684
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 56,060 56,060 14,000 14,000
docs.main -0.03% -0.00% 645,679 645,464 203,065 203,061
packages/material-ui/build/umd/material-ui.production.min.js 0.00% 0.00% 294,911 294,911 84,103 84,103

Generated by 🚫 dangerJS against 6e55b5d

@eps1lon eps1lon self-requested a review May 21, 2019 05:16
@eps1lon eps1lon added the core label May 21, 2019
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.

Nice. At least we can see at the module system if a module is for the server/shared or client.

@eps1lon eps1lon changed the title [core] Remove babel-node [core] Remove babel-node for server/shared modules May 22, 2019
@oliviertassinari oliviertassinari changed the base branch from next to master May 23, 2019 21:07
@oliviertassinari
Copy link
Member

oliviertassinari commented May 24, 2019

I fail to see how this change will provide value (it needs time to migrate, time we could spend on something else, after the change, we will try to write an import, if it doesn't work, we will try a require, I don't know when one should be used over the other, it's inconsistent, do we have a quantification of the overhead, 15%?).

But I don't want to weight more than giving my "feeling" on the topic (I have fewer context than you). We can't improve without undertaking actions.

@eps1lon
Copy link
Member

eps1lon commented May 25, 2019

I fail to see how this change will provide value (it needs time to migrate, time we could spend on something else, after the change, we will try to write an import, if it doesn't work, we will try a require, I don't know when one should be used over the other, it's inconsistent, do we have a quantification of the overhead, 15%?).

Not sure how the time argument applies after the time has been invested. The rest is outlined in the original issue (startup time, debugging).

@eps1lon eps1lon merged commit 3f1bf78 into mui:master May 25, 2019
@eps1lon
Copy link
Member

eps1lon commented May 25, 2019

@cvanem Thanks. Will come in handy later. Direct benefit isn't that obvious but it will pay off.

@zannager zannager added the core Infrastructure work going on behind the scenes label Feb 20, 2023
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.

5 participants