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

Stop using old JSX transform #12253

Merged
merged 1 commit into from Dec 8, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
135 changes: 113 additions & 22 deletions packages/babel-helper-builder-react-jsx-experimental/src/index.js
Expand Up @@ -10,6 +10,31 @@ const DEFAULT = {
};

export function helper(babel, options) {
// TODO (Babel 8): Remove `useBuiltIns` & `useSpread`
const { useSpread = false, useBuiltIns = false } = options;
if (options.runtime === "classic") {
if (typeof useSpread !== "boolean") {
throw new Error(
"transform-react-jsx currently only accepts a boolean option for " +
"useSpread (defaults to false)",
);
}

if (typeof useBuiltIns !== "boolean") {
throw new Error(
"transform-react-jsx currently only accepts a boolean option for " +
"useBuiltIns (defaults to false)",
);
}

if (useSpread && useBuiltIns) {
throw new Error(
"transform-react-jsx currently only accepts useBuiltIns or useSpread " +
"but not both",
);
}
}

const FILE_NAME_VAR = "_jsxFileName";

const JSX_SOURCE_ANNOTATION_REGEX = /\*?\s*@jsxImportSource\s+([^\s]+)/;
Expand Down Expand Up @@ -43,16 +68,16 @@ export function helper(babel, options) {
}
}

const source = t.jsxAttribute(
t.jsxIdentifier("__source"),
t.jsxExpressionContainer(makeSource(path, state)),
);
const self = t.jsxAttribute(
t.jsxIdentifier("__self"),
t.jsxExpressionContainer(t.thisExpression()),
);
const source = t.jsxAttribute(
t.jsxIdentifier("__source"),
t.jsxExpressionContainer(makeSource(path, state)),
);

path.pushContainer("attributes", [source, self]);
path.pushContainer("attributes", [self, source]);
Andarist marked this conversation as resolved.
Show resolved Hide resolved
},
};

Expand Down Expand Up @@ -246,19 +271,22 @@ You can set \`throwIfNamespace: false\` to bypass this warning.`,
}
},

exit(path, state) {
if (
state.get("@babel/plugin-react-jsx/runtime") === "classic" &&
state.get("@babel/plugin-react-jsx/pragmaSet") &&
state.get("@babel/plugin-react-jsx/usedFragment") &&
!state.get("@babel/plugin-react-jsx/pragmaFragSet")
) {
throw new Error(
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this makes much sense - it's overly restrictive. The old transform had no such validation and TS also has never thrown for this situation:
https://www.typescriptlang.org/play?ts=4.1.0-beta#code/PQKhAIAECsGcA9x0SYAoNBuAPAEwJYBuAfNsASRjqcMUA

Copy link
Member

Choose a reason for hiding this comment

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

Can you just comment out this code for now?

I'm not 100% sure that we should remove this check (well, we should now for backward compat, but we might re-introduce it in Babel 8). The question is: should we prioritize for React libraries (e.g. Emotion) or for other "frameworks" (e.g. Preact)?

In the first case it's better not to throw because it should "work by default", while in the second case it's better to "prevent a mistake by default".

Copy link
Member Author

Choose a reason for hiding this comment

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

The question is: should we prioritize for React libraries (e.g. Emotion) or for other "frameworks" (e.g. Preact)?

Yeah, definitely a concern and I don't have a perfect answer for that. My main motivation factors here are that not throwing in this case is:

  • backward-compatible
  • matching how TS behaves

Using pragmas is not a very common use case so I would be inclined to favor backward compatibility, usually, people should really have those things configured globally per project in a config file. Pragmas are mostly useful in configuration-constrained tools.

"transform-react-jsx: pragma has been set but " +
"pragmaFrag has not been set",
);
}
},
// TODO (Babel 8): Decide if this should be removed or brought back.
// see: https://github.com/babel/babel/pull/12253#discussion_r513086528
//
// exit(path, state) {
// if (
// state.get("@babel/plugin-react-jsx/runtime") === "classic" &&
// state.get("@babel/plugin-react-jsx/pragmaSet") &&
// state.get("@babel/plugin-react-jsx/usedFragment") &&
// !state.get("@babel/plugin-react-jsx/pragmaFragSet")
// ) {
// throw new Error(
// "transform-react-jsx: pragma has been set but " +
// "pragmaFrag has not been set",
// );
// }
// },
},
};

Expand Down Expand Up @@ -781,6 +809,7 @@ You can set \`throwIfNamespace: false\` to bypass this warning.`,
}

const attribs = buildCreateElementOpeningElementAttributes(
file,
path,
openingPath.node.attributes,
);
Expand All @@ -804,8 +833,35 @@ You can set \`throwIfNamespace: false\` to bypass this warning.`,
* breaking on spreads, we then push a new object containing
* all prior attributes to an array for later processing.
*/
function buildCreateElementOpeningElementAttributes(path, attribs) {
const props = [];
function buildCreateElementOpeningElementAttributes(file, path, attribs) {
Copy link
Member Author

Choose a reason for hiding this comment

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

changes here are based on the old transform

// TODO (Babel 8): Only leave this branch of the code and remove the rest
if (
RUNTIME_DEFAULT === "automatic" ||
JLHwung marked this conversation as resolved.
Show resolved Hide resolved
file.get("@babel/plugin-react-jsx/runtime") === "automatic"
) {
const props = [];
const found = Object.create(null);

for (const attr of attribs) {
const name =
t.isJSXAttribute(attr) &&
t.isJSXIdentifier(attr.name) &&
attr.name.name;

if (name === "__source" || name === "__self") {
if (found[name]) throw sourceSelfError(path, name);
found[name] = true;
if (!options.development) continue;
}

props.push(convertAttribute(attr));
}

return props.length > 0 ? t.objectExpression(props) : t.nullLiteral();
}

let props = [];
const objs = [];
const found = Object.create(null);

for (const attr of attribs) {
Expand All @@ -820,10 +876,45 @@ You can set \`throwIfNamespace: false\` to bypass this warning.`,
if (!options.development) continue;
}

props.push(convertAttribute(attr));
if (useSpread || !t.isJSXSpreadAttribute(attr)) {
props.push(convertAttribute(attr));
} else {
if (props.length) {
objs.push(t.objectExpression(props));
props = [];
}
objs.push(attr.argument);
}
}

if (!props.length && !objs.length) {
return t.nullLiteral();
}

if (useSpread) {
return props.length > 0 ? t.objectExpression(props) : t.nullLiteral();
}

if (props.length) {
objs.push(t.objectExpression(props));
props = [];
}

return props.length > 0 ? t.objectExpression(props) : t.nullLiteral();
if (objs.length === 1) {
return objs[0];
}

// looks like we have multiple objects
if (!t.isObjectExpression(objs[0])) {
objs.unshift(t.objectExpression([]));
}

const helper = useBuiltIns
? t.memberExpression(t.identifier("Object"), t.identifier("assign"))
: file.addHelper("extends");

// spread it
return t.callExpression(helper, objs);
}

function sourceSelfError(path, name) {
Expand Down
Expand Up @@ -21,12 +21,12 @@ var x = /*#__PURE__*/_jsxDEV(_Fragment, {
columnNumber: 7
}, this), /*#__PURE__*/_createElement("div", { ...props,
key: "4",
__self: this,
__source: {
fileName: _jsxFileName,
lineNumber: 7,
columnNumber: 7
},
__self: this
}
})]
}, void 0, true, {
fileName: _jsxFileName,
Expand Down
@@ -1,42 +1,45 @@
var _jsxFileName = "<CWD>/packages/babel-plugin-transform-react-jsx-development/test/fixtures/linux/classic-runtime/input.js";

function _extends() { _extends = Object.assign || function (target) { for (var i = 1; i < arguments.length; i++) { var source = arguments[i]; for (var key in source) { if (Object.prototype.hasOwnProperty.call(source, key)) { target[key] = source[key]; } } } return target; }; return _extends.apply(this, arguments); }

var x = /*#__PURE__*/React.createElement(React.Fragment, null, /*#__PURE__*/React.createElement("div", {
__self: this,
__source: {
fileName: _jsxFileName,
lineNumber: 3,
columnNumber: 5
},
__self: this
}
}, /*#__PURE__*/React.createElement("div", {
key: "1",
__self: this,
__source: {
fileName: _jsxFileName,
lineNumber: 4,
columnNumber: 9
},
__self: this
}
}), /*#__PURE__*/React.createElement("div", {
key: "2",
meow: "wolf",
__self: this,
__source: {
fileName: _jsxFileName,
lineNumber: 5,
columnNumber: 9
},
__self: this
}
}), /*#__PURE__*/React.createElement("div", {
key: "3",
__self: this,
__source: {
fileName: _jsxFileName,
lineNumber: 6,
columnNumber: 9
},
__self: this
}), /*#__PURE__*/React.createElement("div", { ...props,
}
}), /*#__PURE__*/React.createElement("div", _extends({}, props, {
key: "4",
__self: this,
__source: {
fileName: _jsxFileName,
lineNumber: 7,
columnNumber: 9
},
__self: this
})));
}
}))));
Expand Up @@ -21,12 +21,12 @@ var x = /*#__PURE__*/_jsxDEV(_Fragment, {
columnNumber: 7
}, this), /*#__PURE__*/_createElement("div", { ...props,
key: "4",
__self: this,
__source: {
fileName: _jsxFileName,
lineNumber: 7,
columnNumber: 7
},
__self: this
}
})]
}, void 0, true, {
fileName: _jsxFileName,
Expand Down
@@ -1,42 +1,45 @@
var _jsxFileName = "<CWD>\\packages\\babel-plugin-transform-react-jsx-development\\test\\fixtures\\windows\\classic-runtime-windows\\input.js";

function _extends() { _extends = Object.assign || function (target) { for (var i = 1; i < arguments.length; i++) { var source = arguments[i]; for (var key in source) { if (Object.prototype.hasOwnProperty.call(source, key)) { target[key] = source[key]; } } } return target; }; return _extends.apply(this, arguments); }

var x = /*#__PURE__*/React.createElement(React.Fragment, null, /*#__PURE__*/React.createElement("div", {
__self: this,
__source: {
fileName: _jsxFileName,
lineNumber: 3,
columnNumber: 5
},
__self: this
}
}, /*#__PURE__*/React.createElement("div", {
key: "1",
__self: this,
__source: {
fileName: _jsxFileName,
lineNumber: 4,
columnNumber: 7
},
__self: this
}
}), /*#__PURE__*/React.createElement("div", {
key: "2",
meow: "wolf",
__self: this,
__source: {
fileName: _jsxFileName,
lineNumber: 5,
columnNumber: 7
},
__self: this
}
}), /*#__PURE__*/React.createElement("div", {
key: "3",
__self: this,
__source: {
fileName: _jsxFileName,
lineNumber: 6,
columnNumber: 7
},
__self: this
}), /*#__PURE__*/React.createElement("div", { ...props,
}
}), /*#__PURE__*/React.createElement("div", _extends({}, props, {
key: "4",
__self: this,
__source: {
fileName: _jsxFileName,
lineNumber: 7,
columnNumber: 7
},
__self: this
})));
}
}))));
60 changes: 48 additions & 12 deletions packages/babel-plugin-transform-react-jsx/src/index.js
@@ -1,18 +1,54 @@
/* eslint-disable-next-line @babel/development/plugin-name */
import transformClassic from "./transform-classic";
/* eslint-disable-next-line @babel/development/plugin-name */
import transformAutomatic from "./transform-automatic";
import jsx from "@babel/plugin-syntax-jsx";
import { helper } from "@babel/helper-builder-react-jsx-experimental";
import { declare } from "@babel/helper-plugin-utils";
import { types as t } from "@babel/core";

export default declare((api, options) => {
const { runtime = "classic" } = options;
const PURE_ANNOTATION = options.pure;

// we throw a warning in helper-builder-react-jsx-experimental if runtime
// is neither automatic or classic because we will remove this file
// in v8.0.0
if (runtime === "classic") {
return transformClassic(api, options);
} else {
return transformAutomatic(api, options);
}
const visitor = helper(api, {
Copy link
Member Author

Choose a reason for hiding this comment

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

this is basically just "./transform-automatic" inlined here

pre(state) {
const tagName = state.tagName;
const args = state.args;
if (t.react.isCompatTag(tagName)) {
args.push(t.stringLiteral(tagName));
} else {
args.push(state.tagExpr);
}
},

post(state, pass) {
if (pass.get("@babel/plugin-react-jsx/runtime") === "classic") {
state.createElementCallee = pass.get(
"@babel/plugin-react-jsx/createElementIdentifier",
)();

state.pure =
PURE_ANNOTATION ?? !pass.get("@babel/plugin-react-jsx/pragmaSet");
} else {
state.jsxCallee = pass.get("@babel/plugin-react-jsx/jsxIdentifier")();
state.jsxStaticCallee = pass.get(
"@babel/plugin-react-jsx/jsxStaticIdentifier",
)();
state.createElementCallee = pass.get(
"@babel/plugin-react-jsx/createElementIdentifier",
)();

state.pure =
PURE_ANNOTATION ??
!pass.get("@babel/plugin-react-jsx/importSourceSet");
}
},

...options,
development: false,
runtime,
});

return {
name: "transform-react-jsx",
inherits: jsx,
visitor,
};
});