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

Class features loose should have precedence over preset-env #11634

Merged
merged 6 commits into from May 29, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
95 changes: 65 additions & 30 deletions packages/babel-helper-create-class-features-plugin/src/features.js
Expand Up @@ -8,6 +8,12 @@ export const FEATURES = Object.freeze({
privateIn: 1 << 4,
});

const featuresSameLoose = [
FEATURES.fields,
FEATURES.privateMethods,
FEATURES.privateIn,
];

// We can't use a symbol because this needs to always be the same, even if
// this package isn't deduped by npm. e.g.
// - node_modules/
Expand All @@ -18,49 +24,67 @@ export const FEATURES = Object.freeze({
const featuresKey = "@babel/plugin-class-features/featuresKey";
const looseKey = "@babel/plugin-class-features/looseKey";

// See https://github.com/babel/babel/issues/11622.
// Since preset-env sets loose for the fields and private methods plugins, it can
// cause conflicts with the loose mode set by an explicit plugin in the config.
// To solve this problem, we ignore preset-env's loose mode if another plugin
// explicitly sets it
// The code to handle this logic doesn't check that "low priority loose" is always
// the same. However, it is only set by the preset and not directly by users:
// unless someone _wants_ to break it, it shouldn't be a problem.
const looseLowPriorityKey =
"@babel/plugin-class-features/looseLowPriorityKey/#__internal__@babel/preset-env__please-overwrite-loose-instead-of-throwing";

export function enableFeature(file, feature, loose) {
// We can't blindly enable the feature because, if it was already set,
// "loose" can't be changed, so that
// @babel/plugin-class-properties { loose: true }
// @babel/plugin-class-properties { loose: false }
// is transformed in loose mode.
// We only enabled the feature if it was previously disabled.
if (!hasFeature(file, feature)) {
if (!hasFeature(file, feature) || canIgnoreLoose(file, feature)) {
file.set(featuresKey, file.get(featuresKey) | feature);
if (loose) file.set(looseKey, file.get(looseKey) | feature);
if (
loose ===
"#__internal__@babel/preset-env__prefer-true-but-false-is-ok-if-it-prevents-an-error"

This comment was marked as off-topic.

) {
setLoose(file, feature, true);
file.set(looseLowPriorityKey, file.get(looseLowPriorityKey) | feature);
} else if (
loose ===
"#__internal__@babel/preset-env__prefer-false-but-true-is-ok-if-it-prevents-an-error"
) {
setLoose(file, feature, false);
file.set(looseLowPriorityKey, file.get(looseLowPriorityKey) | feature);
} else {
setLoose(file, feature, loose);
}
}

if (
hasFeature(file, FEATURES.fields) &&
hasFeature(file, FEATURES.privateMethods) &&
isLoose(file, FEATURES.fields) !== isLoose(file, FEATURES.privateMethods)
) {
throw new Error(
"'loose' mode configuration must be the same for both @babel/plugin-proposal-class-properties " +
"and @babel/plugin-proposal-private-methods",
);
}
let resolvedLoose: void | true | false;

if (
hasFeature(file, FEATURES.fields) &&
hasFeature(file, FEATURES.privateIn) &&
isLoose(file, FEATURES.fields) !== isLoose(file, FEATURES.privateIn)
) {
throw new Error(
"'loose' mode configuration must be the same for both @babel/plugin-proposal-class-properties " +
"and @babel/plugin-proposal-private-property-in-object",
);
for (const mask of featuresSameLoose) {
if (!hasFeature(file, mask)) continue;

const loose = isLoose(file, mask);

if (canIgnoreLoose(file, mask)) {
continue;
} else if (resolvedLoose === !loose) {
throw new Error(
"'loose' mode configuration must be the same for @babel/plugin-proposal-class-properties, " +
"@babel/plugin-proposal-private-methods and " +
"@babel/plugin-proposal-private-property-in-object (when they are enabled).",
);
} else {
resolvedLoose = loose;
}
}

if (
hasFeature(file, FEATURES.privateMethods) &&
hasFeature(file, FEATURES.privateIn) &&
isLoose(file, FEATURES.privateMethods) !== isLoose(file, FEATURES.privateIn)
) {
throw new Error(
"'loose' mode configuration must be the same for both @babel/plugin-proposal-private-methods " +
"and @babel/plugin-proposal-private-property-in-object",
);
if (resolvedLoose !== undefined) {
for (const mask of featuresSameLoose) {
if (hasFeature(file, mask)) setLoose(file, mask, resolvedLoose);
}
}
}

Expand All @@ -72,6 +96,17 @@ export function isLoose(file, feature) {
return !!(file.get(looseKey) & feature);
}

function setLoose(file, feature, loose) {
if (loose) file.set(looseKey, file.get(looseKey) | feature);
else file.set(looseKey, file.get(looseKey) & ~feature);

file.set(looseLowPriorityKey, file.get(looseLowPriorityKey) & ~feature);
}

function canIgnoreLoose(file, feature) {
return !!(file.get(looseLowPriorityKey) & feature);
}

export function verifyUsedFeatures(path, file) {
if (hasOwnDecorators(path.node)) {
if (!hasFeature(file, FEATURES.decorators)) {
Expand Down
23 changes: 19 additions & 4 deletions packages/babel-preset-env/src/index.js
Expand Up @@ -301,10 +301,25 @@ export default declare((api, opts) => {

const pluginUseBuiltIns = useBuiltIns !== false;
const plugins = Array.from(pluginNames)
.map(pluginName => [
getPlugin(pluginName),
{ spec, loose, useBuiltIns: pluginUseBuiltIns },
])
.map(pluginName => {
if (
pluginName === "proposal-class-properties" ||
pluginName === "proposal-private-methods"
JLHwung marked this conversation as resolved.
Show resolved Hide resolved
) {
return [
getPlugin(pluginName),
{
loose: loose
? "#__internal__@babel/preset-env__prefer-true-but-false-is-ok-if-it-prevents-an-error"
: "#__internal__@babel/preset-env__prefer-false-but-true-is-ok-if-it-prevents-an-error",
},
];
}
return [
getPlugin(pluginName),
{ spec, loose, useBuiltIns: pluginUseBuiltIns },
];
})
.concat(polyfillPlugins);

if (debug) {
Expand Down
@@ -0,0 +1,5 @@
class A {
x = 2;

#foo() {}
}
@@ -0,0 +1,11 @@
{
"presets": [
["env", {
"targets": { "node": 10 },
"shippedProposals": true
}]
],
"plugins": [
["proposal-private-methods", { "loose": true }]
]
}
@@ -0,0 +1,17 @@
var id = 0;

function _classPrivateFieldLooseKey(name) { return "__private_" + id++ + "_" + name; }

class A {
constructor() {
Object.defineProperty(this, _foo, {
value: _foo2
});
this.x = 2;
}

}

var _foo = _classPrivateFieldLooseKey("foo");

var _foo2 = function _foo2() {};
@@ -0,0 +1,5 @@
class A {
x = 2;

#foo() {}
}
@@ -0,0 +1,9 @@
{
"presets": [
["env", {
"targets": { "node": 10 },
"shippedProposals": true,
"loose": true
}]
]
}
@@ -0,0 +1,17 @@
var id = 0;

function _classPrivateFieldLooseKey(name) { return "__private_" + id++ + "_" + name; }

class A {
constructor() {
Object.defineProperty(this, _foo, {
value: _foo2
});
this.x = 2;
}

}

var _foo = _classPrivateFieldLooseKey("foo");

var _foo2 = function _foo2() {};
@@ -0,0 +1,5 @@
class A {
x = 2;

#foo() {}
}
@@ -0,0 +1,8 @@
{
"presets": [
["env", {
"targets": { "node": 10 },
"shippedProposals": true
}]
]
}
@@ -0,0 +1,14 @@
function _defineProperty(obj, key, value) { if (key in obj) { Object.defineProperty(obj, key, { value: value, enumerable: true, configurable: true, writable: true }); } else { obj[key] = value; } return obj; }

class A {
constructor() {
_foo.add(this);

_defineProperty(this, "x", 2);
}

}

var _foo = new WeakSet();

var _foo2 = function _foo2() {};
@@ -0,0 +1,5 @@
class A {
x = 2;

#foo() {}
}
@@ -0,0 +1,12 @@
{
"presets": [
["env", {
"targets": { "node": 10 },
"shippedProposals": true,
"loose": true
}]
],
"plugins": [
["proposal-class-properties", { "loose": true }]
]
}
@@ -0,0 +1,17 @@
var id = 0;

function _classPrivateFieldLooseKey(name) { return "__private_" + id++ + "_" + name; }

class A {
constructor() {
Object.defineProperty(this, _foo, {
value: _foo2
});
this.x = 2;
}

}

var _foo = _classPrivateFieldLooseKey("foo");

var _foo2 = function _foo2() {};
@@ -0,0 +1,5 @@
class A {
x = 2;

#foo() {}
}
@@ -0,0 +1,11 @@
{
"presets": [
["env", {
"targets": { "node": 10 },
"shippedProposals": true
}]
],
"plugins": [
["proposal-class-properties", { "loose": true }]
]
}
@@ -0,0 +1,17 @@
var id = 0;

function _classPrivateFieldLooseKey(name) { return "__private_" + id++ + "_" + name; }

class A {
constructor() {
Object.defineProperty(this, _foo, {
value: _foo2
});
this.x = 2;
}

}

var _foo = _classPrivateFieldLooseKey("foo");

var _foo2 = function _foo2() {};
@@ -0,0 +1,5 @@
class A {
x = 2;

#foo() {}
}
@@ -0,0 +1,12 @@
{
"presets": [
["env", {
"targets": { "node": 10 },
"shippedProposals": true,
"loose": true
}]
],
"plugins": [
["proposal-class-properties", { "loose": false }]
]
}
@@ -0,0 +1,14 @@
function _defineProperty(obj, key, value) { if (key in obj) { Object.defineProperty(obj, key, { value: value, enumerable: true, configurable: true, writable: true }); } else { obj[key] = value; } return obj; }

class A {
constructor() {
_foo.add(this);

_defineProperty(this, "x", 2);
}

}

var _foo = new WeakSet();

var _foo2 = function _foo2() {};