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

[transform-react-jsx] Add useSpread option to transform JSX #10572

Merged
merged 12 commits into from Oct 29, 2019

Conversation

@ivandevp
Copy link
Contributor

ivandevp commented Oct 18, 2019

Q                       A
Fixed Issues? Fixes #9652
Patch: Bug Fix? No
Major: Breaking Change? No
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link babel/website#2110
Any Dependency Changes? No
License MIT
@buildsize

This comment has been minimized.

Copy link

buildsize bot commented Oct 18, 2019

File name Previous Size New Size Change
babel-preset-env.js 2.73 MB 2.73 MB 9 bytes (0%)
babel-preset-env.min.js 1.65 MB 1.65 MB 9 bytes (0%)
babel.js 2.92 MB 2.92 MB 421 bytes (0%)
babel.min.js 1.61 MB 1.61 MB 245 bytes (0%)
test262.tap 4.84 MB [deleted]
Copy link
Member

nicolo-ribaudo left a comment

Can you also add a test where the spread isn't the first property?

@@ -172,6 +172,14 @@ You can turn on the 'throwIfNamespace' flag to bypass this warning.`,
let _props = [];
const objs = [];

const useSpread = file.opts.useSpread || false;

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Oct 18, 2019

Member

I don't like that this accepts 0, but let's keep it as-is for consistency with the other options of this file 🤷‍♀️

We should revisit it in Babel 8.

This comment has been minimized.

Copy link
@JLHwung

JLHwung Oct 18, 2019

Contributor

IMO, we could begin on useSpread now because it is a new option. useBuiltins: 0 is strange enough and it does not justify that you can even use useSpread: 0.

Copy link
Contributor

JLHwung left a comment

I think when useSpread is enabled, we can further simplify the logic to

return attribs.map(convertAttribute)

because objs is used to support breaking at spreadElement only.

And we may teach convertAttribute to replace JSXSpreadAttribute by spreadElement.

@@ -172,6 +172,14 @@ You can turn on the 'throwIfNamespace' flag to bypass this warning.`,
let _props = [];
const objs = [];

const useSpread = file.opts.useSpread || false;

This comment has been minimized.

Copy link
@JLHwung

JLHwung Oct 18, 2019

Contributor

I think we should also add useSpread to preset-react, too.

Another thought is that may be we should give a warning (or even error) when both useBuiltIns and useSpread are true because useSpread precedes useBuiltIns.

);
}

const useBuiltIns = setDefaultValue(file.opts.useBuiltIns, false);

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Oct 25, 2019

Member

While this makes 100% sense, it is a breaking change (for example, if you pass 0) and we should defer it to Babel 8.

This comment has been minimized.

Copy link
@ivandevp

ivandevp Oct 27, 2019

Author Contributor

So. Should I go back to the previous version?

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Oct 27, 2019

Member

Yes 👍 but only here, not for useSpread.

This comment has been minimized.

Copy link
@ivandevp

ivandevp Oct 27, 2019

Author Contributor

Oh ok, got it.

@@ -172,7 +180,15 @@ You can turn on the 'throwIfNamespace' flag to bypass this warning.`,
let _props = [];
const objs = [];

const useBuiltIns = file.opts.useBuiltIns || false;
const useSpread = setDefaultValue(file.opts.useSpread, false);

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Oct 25, 2019

Member

You can use

const { useSpread = false } = file.opts;
@ivandevp

This comment has been minimized.

Copy link
Contributor Author

ivandevp commented Oct 27, 2019

I think when useSpread is enabled, we can further simplify the logic to

return attribs.map(convertAttribute)

because objs is used to support breaking at spreadElement only.

And we may teach convertAttribute to replace JSXSpreadAttribute by spreadElement.

Hey @JLHwung , do you mean to add a condition before while loop to check if useSpread is set and put an early return mapping all attributes to convertAttribute? If that's the case, I would need to return and object expression instead of an array, right? Something like:

if (useSpread) {
  const props = attribs.map(convertAttribute);
  return t.objectExpression(props);
}
@JLHwung

This comment has been minimized.

Copy link
Contributor

JLHwung commented Oct 27, 2019

@ivandevp correct, I missed the objectExpression but you got me.

@ivandevp

This comment has been minimized.

Copy link
Contributor Author

ivandevp commented Oct 28, 2019

I've made the requested changes guys, let me know if you think we can do any other enhancement.

@JLHwung

This comment has been minimized.

Copy link
Contributor

JLHwung commented Oct 28, 2019

@ivandevp Could you add useSpread to preset-react, too? The other changes look good to me.

@ivandevp

This comment has been minimized.

Copy link
Contributor Author

ivandevp commented Oct 28, 2019

Sure @JLHwung! I forgot that one 😅

@ivandevp

This comment has been minimized.

Copy link
Contributor Author

ivandevp commented Oct 29, 2019

Hey @JLHwung ! I've tried to follow the useBuiltIns flow in preset-react in order to do the same for useSpread. Is my change ok?

@nicolo-ribaudo nicolo-ribaudo added this to the v7.7.0 milestone Oct 29, 2019
Copy link
Member

nicolo-ribaudo left a comment

The CI failure is unrelated

Co-Authored-By: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
@nicolo-ribaudo nicolo-ribaudo merged commit 3d2f365 into babel:master Oct 29, 2019
1 of 2 checks passed
1 of 2 checks passed
test Workflow: test
Details
Travis CI - Pull Request Build Passed
Details
edmorley added a commit to edmorley/neutrino that referenced this pull request Nov 10, 2019
A new `useSpread` option was added in Babel 7.7.0 (babel/babel#10572):
https://babeljs.io/docs/en/babel-preset-react#usespread

Enabling this means that the JSX-> JS conversion will use an inline
object with spread elements (for code that spreads props), rather than
Babel's extend helper or `Object.assign`.

`@babel/env` will still transpile these down if required for the target
browsers (the only reason `useSpread` is not already enabled by default
is that for the upstream project this is a breaking change, since they
cannot assume that users of `babel-preset-react` are always using it
alongside `@babel/env`, even though most likely are).
edmorley added a commit to neutrinojs/neutrino that referenced this pull request Nov 10, 2019
A new `useSpread` option was added in Babel 7.7.0 (babel/babel#10572):
https://babeljs.io/docs/en/babel-preset-react#usespread

Enabling this means that the JSX-> JS conversion will use an inline
object with spread elements (for code that spreads props), rather than
Babel's extend helper or `Object.assign`.

`@babel/env` will still transpile these down if required for the target
browsers (the only reason `useSpread` is not already enabled by default
is that for the upstream project this is a breaking change, since they
cannot assume that users of `babel-preset-react` are always using it
alongside `@babel/env`, even though most likely are).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.