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

Use babel-preset-env and remove transform-runtime #367

Closed
wants to merge 4 commits into from

Conversation

tusbar
Copy link

@tusbar tusbar commented Dec 15, 2017

babel-plugin-transform-runtime factorizes the helpers by using
core-js. This was making this library way too big.

core-js adds 7KB (gzipped) of overhead, while inlining the helpers (even
though some are duplicated) only adds 0.6KB (gzipped).

babel-preset-es2015 is pretty much deprecated, using preset-env allows
to stay up to date with the transforms.

Also remove preset-stage3 to only add transform-object-rest-spread as it
was the only transform necessary.

Finally, remove preset-minify as it was not used.

@tusbar tusbar requested a review from giuseppeg as a code owner December 15, 2017 00:15
babel-plugin-transform-runtime factorizes the helpers by using
core-js. This was making this library way too big.

core-js adds 7KB (gzipped) of overhead, while inlining the helpers (even
though some are duplicated) only adds 0.6KB (gzipped).

babel-preset-es2015 is pretty much deprecated, using preset-env allows
to stay up to date with the transforms.

Also remove preset-stage3 to only add transform-object-rest-spread as it
was the only transform necessary.

Finally, remove preset-minify as it was not used.
@tusbar tusbar force-pushed the remove-transform-runtime branch from 837f528 to 54bbe58 Compare December 15, 2017 00:30
@tusbar
Copy link
Author

tusbar commented Dec 15, 2017

I’m thinking of removing gulp to transpile the code as it really isn’t necessary.

I think that transform-runtime was added only so that gulp and ava could run under babel. The babel conf was shared between the build tools (gulp+ava) and the library itself. This creates confusion and can (and did) impact the build output.

IMHO, the way to go is get rid of gulp, add a npm run build task that does just babel src --out-dir dist and update npm run dev to npm run build -- --watch.

We can then either use babel environments to specify different babel configurations for tests (they need preset-react and [polyfills or transform-runtime]), or like I did in here, specify the babel conf in ava configuration hash.


Also, is it yarn.lock or package-lock.json? 😄

@giuseppeg
Copy link
Collaborator

IMHO, the way to go is get rid of gulp, add a npm run build task that does just babel src --out-dir dist and update npm run dev to npm run build -- --watch.

Yeah, let's do that in another PR though.. so that we can keep the scope of this one small.

yarn.lock or package-lock.json

both, I'll take care of those

@@ -1,3 +1,6 @@
// eslint-disable-next-line import/no-unassigned-import
import 'babel-polyfill'
Copy link
Collaborator

Choose a reason for hiding this comment

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

polyfills are not included in the built library code right?

Copy link
Author

Choose a reason for hiding this comment

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

No, this is only for gulp.

"es2015",
"stage-3",
"react"
"env"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this includes all the stage4 plugins right?

Copy link
Collaborator

@giuseppeg giuseppeg Dec 17, 2017

Choose a reason for hiding this comment

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

Do we need react? We at least need transform-react-jsx for when we publish the library (src/style.js needs to be transpiled)

Copy link
Author

@tusbar tusbar Dec 17, 2017

Choose a reason for hiding this comment

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

babel-preset-env doesn’t include stage-x plugins, it includes all plugins and polyfills necessary for the browserslists config (default is > 1%, last 2 versions, Firefox ESR), which is reasonable I believe.
If people want to support older browsers, they should polyfill themselves (using polyfill.io for example).

preset-react is not needed as src/style.js does not contain any jsx :)

Copy link
Collaborator

@giuseppeg giuseppeg Dec 17, 2017

Choose a reason for hiding this comment

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

preset-react is not needed as src/style.js does not contain any jsx :)

oh right, my bad.

babel-preset-env doesn’t include stage-x plugins, it includes all plugins necessary for the browserslists config (default is > 1%, last 2 versions, Firefox ESR), which is reasonable I believe.
If people want to support older browsers, they should polyfill themselves (using polyfill.io for example).

I am not too concerned about polyfills but syntax. Eg. arrow functions are not supported in IE10 or 9 (it is just an example since IE11 is last 2 version and they will be transpiled anyway). We aim to support what React supports I guess. Can we make sure that preset-env covers the same browsers that are supported right now? ...at list for the client side code :)

Copy link
Author

@tusbar tusbar Dec 17, 2017

Choose a reason for hiding this comment

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

preset-env includes more than preset-es2015 and transpiles to es5. so there shouldn’t be any concerns about that.

from the preset-env doc:

Without any configuration options, babel-preset-env behaves exactly the same as babel-preset-latest (or babel-preset-es2015, babel-preset-es2016, and babel-preset-es2017 together).

es2015 has been deprecated in favor of env.

Copy link
Author

Choose a reason for hiding this comment

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

For example,

export const findStyles = path => {
  if (isStyledJsx(path)) {
    const { node } = path
    return isGlobalEl(node.openingElement) ? [path] : []
  }

  return path.get('children').filter(isStyledJsx)
}

gets transpiled to

var findStyles = exports.findStyles = function findStyles(path) {
  if (isStyledJsx(path)) {
    var node = path.node;

    return isGlobalEl(node.openingElement) ? [path] : [];
  }

  return path.get('children').filter(isStyledJsx);
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ok, yeah I think that without config includes the stage 4 features. If you have a sec maybe you can transpile master and this branch and diff to see whether there are major differences. If not don't worry I'll take a look asap (tomorrow probably) myself.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@giuseppeg giuseppeg Dec 17, 2017

Choose a reason for hiding this comment

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

Thanks man! Looks good, the only issue I see with that is that Map is not polyfilled now. Can we include that polyfill only? I would like to avoid to release a major version only because of this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

get-iterator / Symbols too probably?

@giuseppeg
Copy link
Collaborator

closing in favor of #421 thanks a lot @tusbar !

@giuseppeg giuseppeg closed this Mar 17, 2018
@tusbar tusbar deleted the remove-transform-runtime branch May 3, 2018 14:15
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