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 preset-env target esmodules #7212

Merged
merged 8 commits into from
Jan 22, 2018
Merged

Add preset-env target esmodules #7212

merged 8 commits into from
Jan 22, 2018

Conversation

kristoferbaxter
Copy link
Contributor

@kristoferbaxter kristoferbaxter commented Jan 13, 2018

This PR adds a new feature to Babel's preset-env allowing a developer to target a class of browsers that support <script type="module"> and transpile only what is necessary.

Two important notes:

  1. Specifying a target of esmodules overrides the target field browsers, to avoid conditions where additional specification of browsers would make the bundle increase in size. It expected that one would leverage this feature like https://github.com/kristoferbaxter/preset-env-modules/blob/master/.babelrc.js does... creating a bundle for ESModule only capable browsers, and one for other browsers. Edit: You may continue to override the values specified by browsers and esmodules targets via named browser keys. ({esmodules: true, ie: 10} includes ie 10 in the list).

  2. Leverages CanIUse data by requesting the latest JSON payload for this feature when building new data (similar to compat-table already in use). Compat-table doesn't contain this information, so a different pathway was required to keep the data up to date.

Q                       A
Fixed Issues? none
Patch: Bug Fix? -
Major: Breaking Change? -
Minor: New Feature? Yes!
Tests Added + Pass? Yes, happy to add additional ones as requested
Documentation PR
Any Dependency Changes? Yes, added a dependency for request and on CanIUse for up-to-date data on browsers supporting es modules.
License MIT

@babel-bot
Copy link
Collaborator

babel-bot commented Jan 13, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/6676/

@Andarist
Copy link
Member

Love the idea ❤️ !

@@ -78,6 +78,22 @@ describe("getTargets", () => {
});
});

describe("esmodules", () => {
it("returns browsers supporting modules", () => {
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 add a test that covers the "override browsers" behavior?

Sidenote: should we warn when a user does esmodules and browsers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to add this test. Will do.

Good question on warning: what semantics are the guiding rule for warnings within Babel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this test in 15e2672.

targets.browsers = Object.keys(supportsESModules)
.map(browser => `${browser} ${supportsESModules[browser]}`)
.join(", ");
}
Copy link
Member

Choose a reason for hiding this comment

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

The main worry I'd have with this approach is that, at least for now, all those browsers are very fresh.

But I guess I'll only have to worry about cutting old browsers with this feature in 3~5 years so I guess it's a bit premature to worry.

@Jessidhia
Copy link
Member

Possible future addition: plugin that rewrites import strings into resolved relative paths; it's necessary to actually be able to load these files in browsers.

@existentialism existentialism added PR: New Feature 🚀 A type of pull request used for our changelog categories pkg: preset-env labels Jan 14, 2018
@simonbuchan
Copy link

As a user, here's some bikeshedding:

  • esmodules is confusable with node --experimental-modules.
    Either include the first node with non-experimental modules support or change the name?
  • I would expect it to intersect with existing browser tests, even if no current reasonable value for browsers is not a superset of esmodules: true, that may not be so in the future. At some point, last 2 versions or > 5% will include some browsers with and some browsers without <script module>, and preset-env is here to help cases like that.

@kristoferbaxter
Copy link
Contributor Author

kristoferbaxter commented Jan 16, 2018

I would expect it to intersect with existing browser tests, even if no current reasonable value for browsers is not a superset of esmodules: true, that may not be so in the future. At some point, last 2 versions or > 5% will include some browsers with and some browsers without <script module>, and preset-env is here to help cases like that.

It's likely I am confused about how preset-env is supposed to work. My assumption was Babel users indicated the minimum versions of runtimes they would like to support using preset-env, and this would translate into a set of transpilation features Babel would pass all input through.

In this model, adding a target of {esmodules: true} means something quite specific. This indicates all runtimes capable of supporting ES Modules by default.

As browsers continue to implement features the number of items requiring transpilation under this definition would grow. I believe this makes sense given usage targetting the <script type="module" src="modules.js">/<script nomodule src="nomodules.js"> usecase since you cannot guarantee the device executing the type=module pathway would have any specific user-agent.

Agree this is confusing, and would love some additional options around naming.

Edits: Poor grammar, sorry :(

@nicolo-ribaudo
Copy link
Member

What if, for example, I only want to support Chrome versions which have esmodules support? Something like this should work:

{
  "presets": [["@babel/preset-env", {
    "targets": {
      "esmodules": true,
      "browsers": ["Chrome"]
    }
  }]]

@kristoferbaxter
Copy link
Contributor Author

kristoferbaxter commented Jan 16, 2018

@nicolo-ribaudo Right now that is not supported by this PR. Making esmodules intersect with the browsers field could lead to scenarios where one unintentionally ships code that works only with a subset of the user-agents supporting modules.

I can see a good reason for allowing intersection between the values esmodules targets and the browser field, however this pattern doesn't exist (afaik) in preset-env today.

Would your recommendation be to add this kind of complexity across the board? How would one specify intersection types? i.e start with browsers: ['chrome', 'ie'] and node: 4 but now please intersect by removing transpilation specific to ie 8 etc?

Edit: Here's how one would target Chrome versions targetting esmodules today, and with this change.

presets: [
  [
    '@babel/env', {
      targets: ["Chrome 61"]
    }
  ],
]

@greggb
Copy link

greggb commented Jan 16, 2018

I've had a custom implementation of split module/nomodule bundles on a branch for a while. This would alleviate much of the work that's kept it as a proof of concept.

I wonder if there could be confusion on using this mostly as a gate for "modern browsers" rather than for actual module scripts.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jan 16, 2018

Making esmodules intersect with the browsers field could lead to scenarios where one unintentionally ships code that works only with a subset of the user-agents supporting modules

If, for example, my config is

{
  "targets": {
    "esmodules": true,
    "browsers": ["Chrome 63"]
  }
}

I don't expect Babel to output code which works in Chrome 61, since I explicitly chose Chrome 63.

i.e start with browsers: ['chrome', 'ie'] and node: 4 but now please intersect by removing transpilation specific to ie 8 etc?

Yes. From my understanding, this option means "I will import the file using <script type="module">". preset-env can thus disable plugins only needed for older browsers, since they won't load that script.

I'm not 100% sure of how this would work on node, since it doesn't have <script> tags, but it could work in the same way for two reasons:

  • Consistency with how browsers would be handled
  • It could mean "I will put the output in a .mjs file", since it is the equivalent of <script type="module">.

Here's how one would target Chrome versions targetting esmodules today, and with this change

This would require users to know which browsers support esmodules, which is exactly the duty that preset-env takes away from the developer.


Update: when in the debug mode, preset-env should log the browsers that would be discarded because of esmodules.

@kristoferbaxter
Copy link
Contributor Author

kristoferbaxter commented Jan 16, 2018

@greggb – Hopeful that this path is good for exactly the use-case you presented.

@nicolo-ribaudo – The intersection work you're describing sounds like a great followup to this work. But I'd like to make sure the syntax is well understood.

Here's are two strawman proposals, since afaik preset-env doesn't support intersections between fields listed as targets today.

Target all browsers supporting modules.
(Edge 16, Chrome 61, Safari 10.1, Webkit iOS 10.3)

{
  "targets": {
    "esmodules": true
  }
}

Proposal 1
Target browsers supporting esmodules, but limit beyond that with additional rules.
(Edge 16, Chrome 63, Safari 10.1, Webkit iOS 10.3)

{
  "targets": {
    "esmodules": {
      "browsers": ["Chrome 63"]
    }
  }
}

Proposal 2
Target browsers supporting esmodules, but limit beyond that with additional rules.
(Edge 16, Chrome 63, Safari 10.1, Webkit iOS 10.3)

{
  "targets": {
    "esmodules": true,
    "browsers": ["Chrome 63"]
  }
}

Either of these proposals could work without changing the structure of the current format. This leads me to believe they are good candidates for followup after landing this change.


Edit: Looks like you can already do this override (thanks @simonbuchan and @existentialism), and it works as expected.

{
  "targets": {
    "esmodules": true,
    "chrome": 63
  }
}

@simonbuchan
Copy link

I'd be OK with the current behavior with it also validating you only used one of esmodules and other targets (remember, targets: { ie: 11 } etc. is also a thing) if it's going to effectively discard them? Which can be lifted later when the behavior is specified.

@simonbuchan
Copy link

Here's are two strawman proposals afaik preset-env doesn't support intersections between fields listed as targets today.

That isn't ideal from a learning perspective, but that's because there's only One Right Thing™ - there's no point creating code that will only run in the non-existent intersection of Chrome and Firefox!

It's less obvious why this flag and other targets should only ever be an intersection. Assuming that this flag should only be set if you have another nomodule output with it cleared, then the purpose is to optimize the module output as much as possible. If you are already setting other targets for nomodule, it is because you know it will only run there, and that is still valid for the module bundle, so logically it should use that information to further optimize it.

If you are only targeting module, it could be because you don't care about further optimizations, which is the current PR, but it could be you already have browsers you want to target, but want to ensure that those do support module so you can drop your nomodule bundle.

Regardless, I think the name also matters, and I don't like that it is confusable with top-level module option or that it doesn't imply browsers.

I'm not a huge fan, but how do people feel about scriptType: 'module'?

// .babelrc.js
const browsers = ['last 2 versions'];
module.exports = api => {
  switch (api.env()) { // .env key is deprecated in 7
  default: return { presets: ['env', { targets: { node: true } }] };
  case 'ie': return { presets: ['env', { module: false, targets: { ie: 11 } }] };
  case 'nomodule': return { presets: ['env', { module: false, targets: { browsers } }] };
  case 'module': return { presets: ['env', { module: false, targets: { scriptType: 'module', browsers } }] };
  }
};
  • { scriptType: 'module' }: All browsers with <script type=module> (renamed current PR)
  • { scriptType: 'module', browsers: 'last 2 versions' }: Last 2 versions that also have <script type=module> (renamed Proposal 2)
  • Optionally: { scriptType: 'require module', browsers: 'last 2 versions' }: Error like "IE 11 does not support <script type=module>"

The last one is interesting in that it adds a use for intersecting in browsers (e.g. "last 2 versions except IE"). If that is a use case and browserlist doesn't already support it, then perhaps lifting something like `webpack' Conditions would work?

@existentialism
Copy link
Member

existentialism commented Jan 16, 2018

{
 "targets": {
   "esmodules": true,
   "browsers": ["Chrome 63"]
 }
}

I don't expect Babel to output code which works in Chrome 61, since I explicitly chose Chrome 63.

The current behavior has all other items in targets (chrome: "61", etc) override the results from browsers, and I think it'd be weird/unintuitive if certain items in targets had different behavior from others (intersection, etc).

Also need to consider how the target works alongside other explicit targets, like the following:

{
  "targets": {
    "esmodules": true,
    "chrome": "63"
  }
}

@simonbuchan
Copy link

simonbuchan commented Jan 16, 2018

But the current behavior has all other items in targets (chrome: "61", etc) override the results from browsers, and I think it'd be weird/unintuitive if certain items in targets had different behavior from others (intersection, etc).

Override is intuitive for the browser names targets, as they are clearly more specific than the likely browserlist queries (you could do something bizarre like browsers: ['chrome 50'], chrome: 60 which doesn't have a clear intent, but if you are using both you probably aren't referencing specific browsers in browsers). On the other hand, it doesn't affect node, so it is always unioned with browsers (and other browser fields). So targets already has two ways of combining fields, what's another 😉

@existentialism
Copy link
Member

But all of that can still be explained as "whatever comes out of browsers result is overridden by whatever you set explicitly, node included". I'm not arguing that this is great or the end all be all api/behavior, I'm just stating that this is what we have currently and doing something differently just seems to make it even more confusing.

I mean, we could just as easily move it to a flag outside targets (like uglify -> forceAllTransforms) and have it just override anything placed in targets.

@kristoferbaxter
Copy link
Contributor Author

kristoferbaxter commented Jan 16, 2018

@simonbuchan and @existentialism – Thanks for calling out the targets option for specific browser name/value combinations. I'm creating a test and fixing the issue.

Will update this PR afterwards.

@kristoferbaxter
Copy link
Contributor Author

@existentialism – I've added a test specifically for the case of providing esmodules: true and a keyed browser target ie: 11.

it("returns browser supporting modules and keyed browser overrides", () => {
  ... {
      esmodules: true,
      ie: 11,
  }... );

Also a test for targets: { esmodules: true, browsers: "ie 10", ie: 11 } ensuring that the specification of a browser via named key and version continues to override the "browsers" driven fields browsers and esmodules.

Thanks for pointing this case out! See f2264c3

@jaydenseric
Copy link

jaydenseric commented Jan 17, 2018

Would it be better for the ecosystem to add a new es-modules query option to browserslist?

@simonbuchan
Copy link

simonbuchan commented Jan 17, 2018

browserlist can't do and (except for not which is always and-ed), so you couldn't do the es-modules and chrome 63 case, and I doubt they would accept adding and if they don't have it already....

@simonbuchan
Copy link

Regarding browserlist, the owner replied to a similar question about the websocket feature with "Use caniuse-api", which this PR does.

Looking at it that way, I suppose you could look at this like a features query: features: CanIUseName[], so this would be targets: { ..., features: ['es6-module']. Not sure exactly what else on this list would be useful in the context of preset-env, though? I could maybe imagine building, e.g. a webgl2 vs webgl bundle and want to let babel know the JS it can take advantage of if the webgl2 bundle is loaded...

Still has the issue of how it interacts with browsers, though.

@kristoferbaxter
Copy link
Contributor Author

@existentialism and @simonbuchan – Is there anything else that should be done before this PR can be accepted?

@simonbuchan
Copy link

I personally would like:

  • at least a friendly error if both browsers and esmodules are set for now, and
  • I'm not a fan of the name, it can be confusing compared to the modules option, and doesn't clearly connect to its behavior without reading docs: that said I don't have a much better name: I've currently got scriptType: "module" and moduleLoaders: true, but they're a bit weird.

How do you feel about the features: idea above?

That said, I'm just a guy, "should" doesn't really apply :)

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jan 20, 2018

At least a friendly error if both browsers and esmodules are set for now

👍

I'm not a fan of the name, it can be confusing compared to the modules option, and doesn't clearly connect to its behavior without reading docs: that said I don't have a much better name: I've currently got scriptType: "module" and moduleLoaders: true, but they're a bit weird.

I actually prefer the esmodules name, since scriptType looks too much like the sourceType option of the parser and eslint, and moduleLoaders: true, doesn't really describe what it does.

That said, I'm just a guy, "should" doesn't really apply :)

Your input for this PR is highly appreciated 😉

@yavorsky
Copy link
Member

yavorsky commented Jan 20, 2018

Would it be better for the ecosystem to add a new es-modules query option to browserslist?

It would be cool because ideally, we want browsers to map all targets values. We even had a discussion to add node to browserslist.

If we'll have features field, it could be also extended to has uglify value instead of explicit forceAllTransforms option, but it could complicate targets configuration.

+1 for the esmodules naming, because scriptType presume different values, but we want to handle just 'module'. People could be confused and try nomodule/async, etc values.

@yavorsky
Copy link
Member

yavorsky commented Jan 20, 2018

Also, like an idea.
We already have a PR that adds ecmascript target support.

What if we'll rethink a bit about implementation of these PRs?
For example, we could have something like targets templating. All templates combine and got overridden by the explicitly specified targets. For javascript configs, we could even provide some helpers that could help to control templates merging (overrides to the target's minimum value).

For example:

{
  targets: {
    // will use targets that support es2017 and type="module". maybe some new templates in the future. if both template has same target, then use the lowest one.
    use: ['es2017', 'esmodules'],
    // overrides chrome version to 60.
    chrome: 60,
   // ??? See other idea below
    browsers: '> 2%'
  }
}

The other idea is to use browsers values like other templates, because actually >2% or last 2 chrome versions are kinda templates. Moreover, it won't be unclear how browsers are being merged with esmodules or es2015.

{
  targets: {
    /* will use targets that support es2017, type="module", 
      all browsers with >2% usage and ie11. All templates will be safely merged
      and selected the lowest 
      target. Also, we can extend this with values isn't supported
      by browserslist, like node.
    */
    use: ['es2017', 'esmodules', '>2%', 'ie 11', 'current node'],
    // overrides chrome version to 60.
    chrome: 60,
    // deprecated.
    browsers: ['chrome 60']
  }
}

cc @existentialism @Kovensky @nicolo-ribaudo

@existentialism
Copy link
Member

@yavorsky I'm not opposed to extending browsers to support our own "templates", though I think that's a larger discussion that's beyond the scope of this PR.

What if we land this as is with the addition of an error if esmodules and any other target is used and open a new issue to discuss a more flexible targets syntax that allows for more complex queries?

@yavorsky
Copy link
Member

yavorsky commented Jan 20, 2018

@existentialism Sure!

error if esmodules and any other target is used

Maybe just leave minimal value for conflicts with browsers + show a warning. And override values if explicit targets defined?

@existentialism
Copy link
Member

existentialism commented Jan 20, 2018

@yavorsky works for me /cc: @kristoferbaxter!

@kristoferbaxter
Copy link
Contributor Author

Sounds straightforward, a warning for when browsers target is ignored because esmodules is present.

Will add this over the weekend.

@kristoferbaxter
Copy link
Contributor Author

Messaging added – let's get this merged and I'll modify the repl to include the option!

@existentialism
Copy link
Member

@kristoferbaxter thanks!

Thanks for the great discussion @simonbuchan and everyone! Let's continue in a new issue discussing options for adding a more flexible target syntax?

@existentialism existentialism merged commit b3969d3 into babel:master Jan 22, 2018
@kristoferbaxter kristoferbaxter deleted the preset-modules branch January 22, 2018 22:01
aminmarashi pushed a commit to aminmarashi/babel that referenced this pull request Mar 17, 2018
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 2, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: preset-env PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants