Skip to content

Add babel7 config #1103

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 23 commits into from
Oct 1, 2018
Merged

Add babel7 config #1103

merged 23 commits into from
Oct 1, 2018

Conversation

christianalfoni
Copy link
Contributor

No description provided.

Copy link
Member

@CompuIves CompuIves left a comment

Choose a reason for hiding this comment

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

This is solid, I love the line deletions with it! I left some comments with some questions. I'm mostly curious how this will affect the bundle size and speed(!) compared to babel 6. I remember doing some dark magic with polyfills in https://github.com/CompuIves/codesandbox-client/blob/master/packages/common/load-dynamic-polyfills.js. Might be interesting to look at.

Very nice! Let's merge this in soon 😄.

@@ -55,6 +55,7 @@ module.exports = {
entry: SANDBOX_ONLY
? {
sandbox: [
'@babel/polyfill',
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if this would make the bundler bigger than it needs to be, since with this it's including all babel polyfills out there. Is there a way to make this more fine-grained? I thought that babel-preset-env would do something like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think we only need a setImmediate poly... which can basically just be:

global.setImmediate = global.setImmediate || global.requestAnimationFrame

Will try to remove that and only use custom stuff inside existing polyfill file there

return <LoadedComponent />;
}

return <Loading />;
Copy link
Member

Choose a reason for hiding this comment

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

Can we show the spinner after a set time, like 1s? That will make perceived loading speed faster and is what Loadable has been doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jup, for sure, let me fix :)

@@ -4,33 +4,32 @@ module.exports = {
// Don't try to find .babelrc because we want to force this configuration.
babelrc: false,
presets: [
require.resolve('@babel/preset-flow'),
// Latest stable ECMAScript features
[
'env',
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this 'env' has been unchanged, but that should work too, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, let me fix that, just to be certain it uses the new stuff

@@ -4,33 +4,32 @@ module.exports = {
// Don't try to find .babelrc because we want to force this configuration.
babelrc: false,
presets: [
require.resolve('@babel/preset-flow'),
// Latest stable ECMAScript features
[
'env',
{
targets: {
ie: 11,
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking that we should replace this target with something sensible for CodeSandbox, like 99%>. I will check the analytics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, do that :)

@christianalfoni
Copy link
Contributor Author

christianalfoni commented Sep 25, 2018

Okay, so I had to do some additional work here. Mostly fixing testing. With Babel 7 there are some adjustments to how Jest finds and handles test files.

  • It is supposed to find a babel.config.js file at root of lerna project... which it does. But the pattern matching for node_modules does not work for some reason. I believe this is a bug. This is fixed by having a babel.config.js file in each package requiring it

  • The pattern matching for node_modules has been changed, strengthening my point above, because I had to add a file matcher as well at the end

  • I had to install old babel core version 7.bridge, to allow new babel 7 to run with Jest

  • There was a need to update snapshots, due to returning a SET instead of an ARRAY. This could maybe be related to React update. The content of the collection is the same, so let it through

It really is insane how difficult it is to maintain these large projects, but I think this move it very important. When you get the "insides" on babel 7 as well there is a lot of cleanup that can be done!

Hopefully the last fix is moving in now. Just add the babel 7 bridge thingy as a dep, while it was also a devDep :)

@christianalfoni
Copy link
Contributor Author

Not sure what happens on the integration tests there? Any way to run that locally? Or maybe it is related to PR deployment error there? Maybe I broke some production config. Could you help me find some error message? :)

@christianalfoni christianalfoni force-pushed the babel7 branch 2 times, most recently from 7b92de1 to 4bac6c4 Compare September 27, 2018 18:53
@CompuIves CompuIves changed the title WIP: Add babel7 config Add babel7 config Oct 1, 2018
@CompuIves CompuIves merged commit fedfbe3 into master Oct 1, 2018
mike-north pushed a commit to mike-north/codesandbox-client that referenced this pull request Oct 1, 2018
@SaraVieira SaraVieira deleted the babel7 branch May 14, 2019 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants