Skip to content
Please note that GitHub no longer supports Internet Explorer.

We recommend upgrading to the latest Microsoft Edge, Google Chrome, or Firefox.

Learn more
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

Extract targets parser and compat data from preset-env #10899

Merged
merged 4 commits into from Jan 10, 2020

Conversation

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Dec 20, 2019

Q                       A
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This is PR only moves code around, to expose as public packages internal API which are already used by some Babel integrations:

I created two new packages: @babel/compat-data and @babel/helper-compilation-targets.
I wasn't sure about where to put the info about overlapping plugins which will be useful for preset-modules (I put it in @babel/compat-data) and about the mappings from transform plugins to syntax plugins (I left it in @babel/preset-env).

@nicolo-ribaudo nicolo-ribaudo added this to the v7.8.0 milestone Dec 20, 2019
@nicolo-ribaudo nicolo-ribaudo requested a review from JLHwung Dec 20, 2019
@loganfsmyth

This comment has been minimized.

Copy link
Member

loganfsmyth commented Dec 21, 2019

I swear at this point we should just append a random UUID to every filename except index.js on publish :P

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Dec 21, 2019

Did you see #10850? 😛

@loganfsmyth

This comment has been minimized.

Copy link
Member

loganfsmyth commented Dec 21, 2019

Hah, nope! Had no idea that was a thing Node allowed!

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:env-targets-parser branch 3 times, most recently from b842a7c to fad046a Dec 25, 2019
@@ -7,7 +7,7 @@ const flattenDeep = require("lodash/flattenDeep");
const isEqual = require("lodash/isEqual");
const mapValues = require("lodash/mapValues");
const pickBy = require("lodash/pickBy");
const unreleasedLabels = require("../data/unreleased-labels");
const { unreleasedLabels } = require("../../babel-helper-compilation-targets");

This comment has been minimized.

Copy link
@JLHwung

JLHwung Jan 8, 2020

Contributor

Can we just require @babel/helper-compilation-targets?

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jan 8, 2020

Author Member

Good catch; I'll update it. Note that this file isn't published to npm so it isn't a problem, but using @babel/helper-compilation-targets is more clear.

"version": "0.0.0",
"author": "The Babel Team (https://babeljs.io/team)",
"license": "MIT",
"description": "",

This comment has been minimized.

Copy link
@JLHwung

JLHwung Jan 8, 2020

Contributor

Maybe "Engine compat data used in @babel/preset-env"?

@existentialism

This comment has been minimized.

Copy link
Member

existentialism commented Jan 8, 2020

Any reason to not also move isPluginRequired as another primitive to the helper?

In fact, erring on the side of less packages to maintain, can we just have one package that exports getTargets, our plugin/target data and something like:

isRequired(
  name: string,
  targetEnvironments: Targets,
  options: {
    compatData?: { [key in string]: Targets }
    // ...
  }
) { ... }

This seems like a key use-case (as both vue/ember use it), and would allow anyone to pass their own mappings in, if they wanted.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Jan 8, 2020

Any reason to not also move isPluginRequired as another primitive to the helper?

Maybe I forgot it, I'll check.

In fact, erring on the side of less packages to maintain

I don't think that this should be a goal? The maintenance cost of 2 packages in a monorepo is exactly the same as 1 package.

I created two separate packages because, if we are going to disable sub-package imports in Babel 8.0.0, @babel/compat-data will already be an exception when you can import internal files. If we merge it with the helper package, then some internal files will be importable while others won't, and I think that it would be more confusing.

import type { Targets } from "./types";
import { isUnreleasedVersion, semverify } from "./utils";

export function isItemRequired(supportedEnvironments: Targets, item: Targets) {

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jan 8, 2020

Author Member

Oh, I already extracted it. I called it isItemRequired because it's not just used for plugins, but also for polyfills. Maybe I should choose another name?

This comment has been minimized.

Copy link
@existentialism

existentialism Jan 8, 2020

Member

Ah missed that. I think the name is fine.

@JLHwung
JLHwung approved these changes Jan 8, 2020
@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:env-targets-parser branch from 457871b to ef4edea Jan 8, 2020
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Jan 8, 2020

Whops I accidentally deleted my comment.

I wrote that I agree that the proposed isRequired function would be used both by ember and vue. However, I still think that the data should stay separated in a "data-only" package for direct usage.

Also, do you think that isRequired should default to using the plugins and core-js 2 data, or just the plugins data? And then there is also core-js 3, which keeps its data in a separate package.
(I think that it should only use plugins data)

@existentialism

This comment has been minimized.

Copy link
Member

existentialism commented Jan 9, 2020

The maintenance cost of 2 packages in a monorepo is exactly the same as 1 package.

I'm not sure I agree with that... perhaps mechanically, but I think we should always consider the balance between a package's public API vs. adding more packages.

Also, do you think that isRequired should default to using the plugins and core-js 2 data, or just the plugins data? And then there is also core-js 3, which keeps its data in a separate package.

I think we can go with just plugins data for now.

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:env-targets-parser branch from 06652c9 to 58d6106 Jan 9, 2020
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Jan 9, 2020

@existentialism I ended up not exporting isItemRequired (which I also renamed) to avoid confusion with isRequired.

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:env-targets-parser branch from 58d6106 to 4fd4151 Jan 9, 2020
@nicolo-ribaudo nicolo-ribaudo merged commit 04354d1 into babel:master Jan 10, 2020
4 of 5 checks passed
4 of 5 checks passed
build (13.x)
Details
test262 Workflow: test262
Details
Travis CI - Pull Request Build Passed
Details
build-standalone Workflow: build-standalone
Details
e2e Workflow: e2e
Details
@nicolo-ribaudo nicolo-ribaudo deleted the nicolo-ribaudo:env-targets-parser branch Jan 10, 2020
andersk added a commit to andersk/babel that referenced this pull request Jan 11, 2020
It was moved in babel#10899.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
andersk added a commit to andersk/babel that referenced this pull request Jan 11, 2020
It was moved in babel#10899.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
andersk added a commit to andersk/babel-website that referenced this pull request Jan 11, 2020
It was moved in babel/babel#10899.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
JLHwung added a commit that referenced this pull request Jan 11, 2020
It was moved in #10899.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
sodatea added a commit to sodatea/vue-cli that referenced this pull request Feb 3, 2020
sodatea added a commit to vuejs/vue-cli that referenced this pull request Feb 3, 2020
…abel 8 (#5133)

* refactor: use babel.loadPartialConfigSync (added in babel 7.8)

As planned in babel/babel#10746,
in babel 8 the old `loadPartialConfig` can't be used synchronously.

* refactor: remove dependence on internal babel files, preparing for babel 8

See
babel/babel#10746
babel/babel#10899
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.