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

Add @babel/runtime as a dependency to @wordpress/components #8057

Merged
merged 1 commit into from
Jul 19, 2018

Conversation

gwwar
Copy link
Contributor

@gwwar gwwar commented Jul 19, 2018

Currently we publish a bundle that refers to @babel/runtime but does not require it as an explicit dependency in package.json. To not confuse consumers of this package, let's declare this dependency!

@gwwar gwwar self-assigned this Jul 19, 2018
@gwwar gwwar requested review from youknowriad and gziolo July 19, 2018 18:44
@gwwar gwwar added the [Package] Components /packages/components label Jul 19, 2018
@gwwar gwwar force-pushed the update/components-dependencies branch from 1db2ab6 to 6735aad Compare July 19, 2018 18:48
@gwwar gwwar changed the title Add babel-runtime as a dependency to @wordpress/components Add @babel/runtime as a dependency to @wordpress/components Jul 19, 2018
@gwwar gwwar force-pushed the update/components-dependencies branch from 6735aad to 78ec603 Compare July 19, 2018 18:50
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Yeah, it wasn’t clear how we should approach it. It seems like the best way to go. I think we should do it for every package which transpiles code 😃

@mmtr
Copy link
Contributor

mmtr commented Jul 19, 2018

Yeah, it wasn’t clear how we should approach it. It seems like the best way to go. I think we should do it for every package which transpiles code 😃

agreed! that will also fix #7936

@mmtr
Copy link
Contributor

mmtr commented Jul 19, 2018

@gwwar @gziolo shouldn't we increase the package version?

@gwwar
Copy link
Contributor Author

gwwar commented Jul 19, 2018

shouldn't we increase the package version?

@gziolo Does Lerna bump the package version, or would you like me to update it in package.json?

@gwwar
Copy link
Contributor Author

gwwar commented Jul 19, 2018

I can make PRs for the other packages too, I wanted to see what folks thought first 👍

@gziolo
Copy link
Member

gziolo commented Jul 19, 2018

No, Lerna takes care of it when doing publishing. It also creates git tags and temporary replaces all locally scoped packages with the proper published version in the dependencies list. Isn’t it sweet? 😃

@gwwar
Copy link
Contributor Author

gwwar commented Jul 19, 2018

👍 Thanks for the reviews!

@gwwar gwwar merged commit 635fa82 into master Jul 19, 2018
@gwwar gwwar deleted the update/components-dependencies branch July 19, 2018 19:56
@gziolo
Copy link
Member

gziolo commented Jul 19, 2018

I can make PRs for the other packages too, I wanted to see what folks thought first 👍

That would be ace, I would be able to publish updates tomorrow morning.

@blowery
Copy link
Contributor

blowery commented Jul 20, 2018

I think we should do it for every package which transpiles code

@gziolo At least anything using transform-runtime, yeah.

It looks like we're currently shipping polyfills in the packages. We may not want to do that long term. We're polluting the global prototypes for anything that uses these packages and polyfillng even in cases where the polyfill isn't needed.

Other projects (React, Draft, ...) are now stating what needs to be polyfilled up front and leaving it up to the client. That might be an approach to take?

@gziolo gziolo added this to the 3.3 milestone Jul 20, 2018
@gziolo
Copy link
Member

gziolo commented Jul 20, 2018

It looks like we're currently shipping polyfills in the packages. We may not want to do that long term. We're polluting the global prototypes for anything that uses these packages and polyfillng even in cases where the polyfill isn't needed.

Other projects (React, Draft, ...) are now stating what needs to be polyfilled up front and leaving it up to the client. That might be an approach to take?

Honestly speaking, we are learning the process ourselves. If you have exact recommendations on how to setup all that let us know. Excluding all the packages which aren't transpiled we ship 2 versions using the following Babel config:

  1. build which contains code for the majority of browsers with modules set to commonjs and a new Babel 7 value for useBuiltIns option: usage.
  2. build-module which differs only in a way that it generates ES Harmony modules (import & export).

The fact that we include import statements for both polyfills and Babel runtime comes from the new useBuiltIns setup. Now the questions is how would you see this improved to ensure that consumers can pick ES5 or ESNext version of the code depending on their needs.

/cc @youknowriad

@blowery
Copy link
Contributor

blowery commented Aug 8, 2018

Now the questions is how would you see this improved to ensure that consumers can pick ES5 or ESNext version of the code depending on their needs.

Clients won't generally ever want to consume es.next, it's just too difficult given current tooling. Some will want es5 wrapped in commonJS (package:main) and some will want es5 wrapped in es6 modules (package:module). Some folks are exposing the raw source under jsnext:main but it's not that common as you also have to expose your transpilation expectation in some fashion.

Whether or not you have polyfills in the es5 code is a separate issue. Libraries should not ship polyfills that affect the global scope. Instead you should use babel's transform-runtime to ponyfill usages that need back-compat, or ship the problematic usage and explicitly declare that clients must polyfill certain things it they want the code to work on older browsers. transform-runtime has the limitation that it cannot ponyfill usage of new properties on prototypes, so things like Array.prototype.find won't get fixed. If you go that route, you have to avoid usage (which we did on Calypso for a long time).

@gwwar
Copy link
Contributor Author

gwwar commented Aug 8, 2018

@blowery do you mind opening up a new issue so folks can discuss transformation options for packages? It's a good discussion to have and I'm afraid your comment will be lost on this PR.

@gziolo
Copy link
Member

gziolo commented Aug 9, 2018

We use transform-runtime when producing a distribution version of packages. See:
https://github.com/WordPress/gutenberg/blob/master/packages/babel-preset-default/index.js#L23

We also add @babel/runtime-corejs2 as a dependency of each package which is transpiled, e.g.:
https://github.com/WordPress/gutenberg/blob/master/packages/blocks/package.json#L23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants