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

JSX: Use <> Fragments with transforms #15261

Merged
merged 2 commits into from
May 7, 2019
Merged

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Apr 30, 2019

Description

#15120 adds Fragment handling to @wordpress/babel-plugin-import-jsx-pragma and prepares @wordpress/babel-preset-default for the WordPress environment.

Leverage the <></> automatic Fragment import provided by the babel transform included in the preset. Follow-up to #15120.

How has this been tested?

Inspect transformed code to ensure imports and JSX transforms are working correctly.
Smoke test code using <></>.

Types of changes

Janitorial.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@sirreal sirreal added [Type] Task Issues or PRs that have been broken down into an individual action to take [Status] Blocked Used to indicate that a current effort isn't able to move forward [Package] Block library /packages/block-library labels Apr 30, 2019
@sirreal sirreal self-assigned this Apr 30, 2019
@sirreal sirreal changed the title Update/use empty jsx fragments JSX: Use <> Fragments with transforms Apr 30, 2019
@sirreal sirreal force-pushed the update/use-empty-jsx-fragments branch 2 times, most recently from 17f8101 to c34aed0 Compare May 4, 2019 14:08
@sirreal sirreal marked this pull request as ready for review May 4, 2019 14:16
@sirreal sirreal added Needs Technical Feedback Needs testing from a developer perspective. and removed [Status] Blocked Used to indicate that a current effort isn't able to move forward labels May 4, 2019
@gziolo
Copy link
Member

gziolo commented May 6, 2019

I like how it simplifies the code by removing the need to import Fragment explicitly 👍

I'm wondering whether it should be enforced to use <> in Gutenberg from now on with ESLint rule?

Another question is, how we can communicate in docs that both createElement and Fragment are handled behind the scenes when you use the recommended Babel plugin. /cc @mkaz and @nosolosw

@sirreal
Copy link
Member Author

sirreal commented May 6, 2019

I'm wondering whether it should be enforced to use <> in Gutenberg from now on with ESLint rule?

I do think this syntax should be preferred, however there are still places where <Fragment key={ key }> is necessarily used because <> does not support keys. Any rule would need to be smart enough to discern. In this PR I left files including Fragments with and without keys to use the full <Fragment> in all cases.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

There's a merge conflict to be addressed. It otherwise appears in good order. The specific syntax is a bit off-putting to me (vs. the "explicit" Fragment), but I sense it's a matter of becoming accustomed to, and can be reinforced as a pattern increasingly common to any React project.

@sirreal sirreal force-pushed the update/use-empty-jsx-fragments branch from c34aed0 to da935a6 Compare May 7, 2019 05:40
@sirreal sirreal merged commit 39f568f into master May 7, 2019
@sirreal sirreal deleted the update/use-empty-jsx-fragments branch May 7, 2019 06:05
@sirreal sirreal removed the Needs Technical Feedback Needs testing from a developer perspective. label May 7, 2019
@gziolo
Copy link
Member

gziolo commented May 7, 2019

The specific syntax is a bit off-putting to me (vs. the "explicit" Fragment), but I sense it's a matter of becoming accustomed to, and can be reinforced as a pattern increasingly common to any React project.

I share the same sentiment, in general, it should simplify everything :)

@youknowriad youknowriad added this to the 5.7 (Gutenberg) milestone May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block library /packages/block-library [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants