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.

Sign up for GitHub

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

Babel plugin JSX: Implement Fragment handling #15120

Merged
merged 7 commits into from
Apr 30, 2019
Merged

Babel plugin JSX: Implement Fragment handling #15120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6f5fa88
Implement Fragment handling
sirreal Apr 23, 2019
13a2d6f
Document scopeVariableFrag
sirreal Apr 23, 2019
635798d
Add changelog entry
sirreal Apr 23, 2019
1ec0ebf
Drop handling of named <Fragment />
sirreal Apr 23, 2019
c6ae6b4
Update babel preset to handle JSX Fragments
sirreal Apr 24, 2019
36faddf
Remove bad warn on isDefault with scopeVariableFrag
sirreal Apr 30, 2019
9e13d18
Drop redundant conditional
sirreal Apr 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion packages/babel-plugin-import-jsx-pragma/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## 2.1.0 (unreleased)

### New Feature

- Add Fragment import handling ([#15120](https://github.com/WordPress/gutenberg/pull/15120)).

## 2.0.0 (2019-03-06)

### Breaking Change
Expand All @@ -6,7 +12,7 @@

### Enhancement

- Plugin skips now adding import JSX pragma when the scope variable is defined for all JSX elements ([#13809](https://github.com/WordPress/gutenberg/pull/13809)).
- Plugin skips now adding import JSX pragma when the scope variable is defined for all JSX elements ([#13809](https://github.com/WordPress/gutenberg/pull/13809)).

## 1.1.0 (2018-09-05)

Expand Down
12 changes: 11 additions & 1 deletion packages/babel-plugin-import-jsx-pragma/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,13 @@ module.exports = {
plugins: [
[ '@wordpress/babel-plugin-import-jsx-pragma', {
scopeVariable: 'createElement',
scopeVariableFrag: 'Fragment',
source: '@wordpress/element',
isDefault: false,
} ],
[ '@babel/transform-react-jsx', {
pragma: 'createElement',
pragmaFrag: 'Fragment',
} ],
],
};
Expand All @@ -60,6 +62,13 @@ _Type:_ String

Name of variable required to be in scope for use by the JSX pragma. For the default pragma of React.createElement, the React variable must be within scope.

### `scopeVariableFrag`
sirreal marked this conversation as resolved.
Show resolved Hide resolved

_Type:_ String

Name of variable required to be in scope for `<></>` `Fragment` JSX. Named `<Fragment />` elements
expect Fragment to be in scope and will not add the import.

### `source`

_Type:_ String
Expand All @@ -70,6 +79,7 @@ The module from which the scope variable is to be imported when missing.

_Type:_ Boolean

Whether the scopeVariable is the default import of the source module.
Whether the scopeVariable is the default import of the source module. Note that this has no impact
on `scopeVariableFrag`.

<br/><br/><p align="center"><img src="https://s.w.org/style/images/codeispoetry.png?1" alt="Code is Poetry." /></p>
80 changes: 53 additions & 27 deletions packages/babel-plugin-import-jsx-pragma/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,20 @@
/**
* Default options for the plugin.
*
* @property {string} scopeVariable Name of variable required to be in scope
* for use by the JSX pragma. For the default
* pragma of React.createElement, the React
* variable must be within scope.
* @property {string} source The module from which the scope variable
* is to be imported when missing.
* @property {boolean} isDefault Whether the scopeVariable is the default
* import of the source module.
* @property {string} scopeVariable Name of variable required to be in scope
* for use by the JSX pragma. For the default
* pragma of React.createElement, the React
* variable must be within scope.
* @property {string} scopeVariableFrag Name of variable required to be in scope
* for use by the Fragment pragma.
* @property {string} source The module from which the scope variable
* is to be imported when missing.
* @property {boolean} isDefault Whether the scopeVariable is the default
* import of the source module.
*/
const DEFAULT_OPTIONS = {
scopeVariable: 'React',
scopeVariableFrag: null,
Copy link
Member

Choose a reason for hiding this comment

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

So a consumer must opt in to this? Can there not be a default, like there is with @babel/plugin-transform-react-jsx ?

Is the expected behavior when this is null to not do anything when it encounters a fragment?

Copy link
Member Author

@sirreal sirreal Apr 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think null matches the default expectations across the board. When it's null and <></> Fragments are encountered, no named import is added. However, this is JSX so the scopeVariable will be added.

If we consider the defaults (they could be omitted here):

const webpackConfig = { // …
plugins: [
[
	require.resolve( '@wordpress/babel-plugin-import-jsx-pragma' ),
	{
		scopeVariable: 'React',
		scopeVariableFrag: null,
		source: 'react',
		isDefault: true,
	},
],
[ require.resolve( '@babel/plugin-transform-react-jsx' ), {
	pragma: 'React.createElement',
	pragmaFrag: 'React.Fragment',
} ],
};

Note that pragmaFrag's default is React.Fragment. With default arguments, <>In a Fragment</> will result in something like:

import React from 'react';
React.createElement( React.Fragment, "In a Fragment" );

Copy link
Member

Choose a reason for hiding this comment

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

Oh! That's quite tricky to follow, but it makes sense. Thanks for explaining.

source: 'react',
isDefault: true,
};
Expand Down Expand Up @@ -39,40 +42,63 @@ module.exports = function( babel ) {

return {
visitor: {
JSXElement( path, state ) {
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 explain why this was changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I noticed that without this change, the following test fails (added in this PR):

it( 'adds import for scope variable for Fragments', () => {
	const original = 'let foo = <></>;';
	const string = getTransformedCode( original );

	expect( string ).toBe( 'import React from "react";\nlet foo = <></>;' );
} );
  ● babel-plugin-import-jsx-pragma › adds import for scope variable for Fragments

    expect(received).toBe(expected) // Object.is equality

    Expected: "import React from \"react\";
    let foo = <></>;"
    Received: "let foo = <></>;"

      27 |
      28 |
    > 29 |              expect( string ).toBe( 'import React from "react";\nlet foo = <></>;' );
         |                               ^
      30 |      } );
      31 |
      32 |      it( 'does nothing if there is no jsx', () => {

      at Object.toBe (packages/babel-plugin-import-jsx-pragma/test/index.js:29:20)

It seems that JSX is an alias for most or all JSX that may be encountered.

It seems that <></> should cause the import to be added.

JSX( path, state ) {
if ( state.hasUndeclaredScopeVariable ) {
return;
}

const { scopeVariable } = getOptions( state );
state.hasUndeclaredScopeVariable = ! path.scope.hasBinding( scopeVariable );
},
JSXFragment( path, state ) {
if ( state.hasUndeclaredScopeVariableFrag ) {
return;
}

const { scopeVariableFrag } = getOptions( state );
if ( scopeVariableFrag === null ) {
return;
}

state.hasUndeclaredScopeVariableFrag = ! path.scope.hasBinding( scopeVariableFrag );
},
Program: {
exit( path, state ) {
if ( ! state.hasUndeclaredScopeVariable ) {
return;
}
const { scopeVariable, scopeVariableFrag, source, isDefault } = getOptions( state );

const { scopeVariable, source, isDefault } = getOptions( state );
let scopeVariableSpecifier;
let scopeVariableFragSpecifier;

let specifier;
if ( isDefault ) {
specifier = t.importDefaultSpecifier(
t.identifier( scopeVariable )
);
} else {
specifier = t.importSpecifier(
t.identifier( scopeVariable ),
t.identifier( scopeVariable )
if ( state.hasUndeclaredScopeVariable ) {
if ( isDefault ) {
scopeVariableSpecifier = t.importDefaultSpecifier( t.identifier( scopeVariable ) );
} else {
scopeVariableSpecifier = t.importSpecifier(
t.identifier( scopeVariable ),
t.identifier( scopeVariable )
);
}
}

if ( state.hasUndeclaredScopeVariableFrag ) {
scopeVariableFragSpecifier = t.importSpecifier(
t.identifier( scopeVariableFrag ),
t.identifier( scopeVariableFrag )
);
}

const importDeclaration = t.importDeclaration(
[ specifier ],
t.stringLiteral( source )
);
const importDeclarationSpecifiers = [
scopeVariableSpecifier,
scopeVariableFragSpecifier,
].filter( Boolean );
if ( importDeclarationSpecifiers.length ) {
const importDeclaration = t.importDeclaration(
importDeclarationSpecifiers,
t.stringLiteral( source )
);

path.unshiftContainer( 'body', importDeclaration );
path.unshiftContainer( 'body', importDeclaration );
}
},
},
},
Expand Down
99 changes: 84 additions & 15 deletions packages/babel-plugin-import-jsx-pragma/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,6 @@ import { transformSync } from '@babel/core';
import plugin from '../';

describe( 'babel-plugin-import-jsx-pragma', () => {
function getTransformedCode( source, options = {} ) {
const { code } = transformSync( source, {
configFile: false,
plugins: [
[ plugin, options ],
'@babel/plugin-syntax-jsx',
],
} );

return code;
}

it( 'does nothing if there is no jsx', () => {
const original = 'let foo;';
const string = getTransformedCode( original );
Expand Down Expand Up @@ -49,6 +37,13 @@ describe( 'babel-plugin-import-jsx-pragma', () => {
expect( string ).toBe( 'import React from "react";\n' + original );
} );

it( 'adds import for scope variable for Fragments', () => {
const original = 'let foo = <></>;';
const string = getTransformedCode( original );

expect( string ).toBe( 'import React from "react";\nlet foo = <></>;' );
} );

it( 'allows options customization', () => {
const original = 'let foo = <bar />;';
const string = getTransformedCode( original, {
Expand All @@ -61,7 +56,8 @@ describe( 'babel-plugin-import-jsx-pragma', () => {
} );

it( 'adds import for scope variable even when defined inside the local scope', () => {
const original = 'let foo = <bar />;\n\nfunction local() {\n const createElement = wp.element.createElement;\n}';
const original =
'let foo = <bar />;\n\nfunction local() {\n const createElement = wp.element.createElement;\n}';
const string = getTransformedCode( original, {
scopeVariable: 'createElement',
source: '@wordpress/element',
Expand All @@ -72,20 +68,93 @@ describe( 'babel-plugin-import-jsx-pragma', () => {
} );

it( 'does nothing if the outer scope variable is already defined when using custom options', () => {
const original = 'const {\n createElement\n} = wp.element;\nlet foo = <bar />;';
const original =
'const {\n createElement,\n Fragment\n} = wp.element;\nlet foo = <><bar /></>;';
const string = getTransformedCode( original, {
scopeVariable: 'createElement',
scopeVariableFrag: 'Fragment',
source: '@wordpress/element',
isDefault: false,
} );

expect( string ).toBe( original );
} );

it( 'adds only Fragment when required', () => {
const original = 'const {\n createElement\n} = wp.element;\nlet foo = <><bar /></>;';
const string = getTransformedCode( original, {
scopeVariable: 'createElement',
scopeVariableFrag: 'Fragment',
source: '@wordpress/element',
isDefault: false,
} );

expect( string ).toBe(
'import { Fragment } from "@wordpress/element";\nconst {\n createElement\n} = wp.element;\nlet foo = <><bar /></>;'
);
} );

it( 'adds only createElement when required', () => {
const original = 'const {\n Fragment\n} = wp.element;\nlet foo = <><bar /></>;';
const string = getTransformedCode( original, {
scopeVariable: 'createElement',
scopeVariableFrag: 'Fragment',
source: '@wordpress/element',
isDefault: false,
} );

expect( string ).toBe(
'import { createElement } from "@wordpress/element";\nconst {\n Fragment\n} = wp.element;\nlet foo = <><bar /></>;'
);
} );

it( 'does nothing if the inner scope variable is already defined when using custom options', () => {
const original = '(function () {\n const {\n createElement\n } = wp.element;\n let foo = <bar />;\n})();';
const original =
'(function () {\n const {\n createElement\n } = wp.element;\n let foo = <bar />;\n})();';
const string = getTransformedCode( original, {
scopeVariable: 'createElement',
scopeVariableFrag: 'Fragment',
source: '@wordpress/element',
isDefault: false,
} );

expect( string ).toBe( original );
} );

it( 'adds Fragment as for <></>', () => {
const original = 'let foo = <><bar /><baz /></>;';
const string = getTransformedCode( original, {
scopeVariable: 'createElement',
scopeVariableFrag: 'Fragment',
source: '@wordpress/element',
isDefault: false,
} );

expect( string ).toBe(
'import { createElement, Fragment } from "@wordpress/element";\nlet foo = <><bar /><baz /></>;'
);
} );

it( 'does not add Fragment import for <Fragment />', () => {
const original = 'let foo = <Fragment><bar /><baz /></Fragment>;';
const string = getTransformedCode( original, {
scopeVariable: 'createElement',
scopeVariableFrag: 'Fragment',
source: '@wordpress/element',
isDefault: false,
} );

expect( string ).toBe(
'import { createElement } from "@wordpress/element";\nlet foo = <Fragment><bar /><baz /></Fragment>;'
);
} );
} );

function getTransformedCode( source, options = {} ) {
const { code } = transformSync( source, {
configFile: false,
plugins: [ [ plugin, options ], '@babel/plugin-syntax-jsx' ],
} );

return code;
}
6 changes: 6 additions & 0 deletions packages/babel-preset-default/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## 4.1.0 (unreleased)

### New Feature

- Handle `<></>` JSX Fragments with `@wordpress/element` `Fragment` ([#15120](https://github.com/WordPress/gutenberg/pull/15120)).

## 4.0.0 (2019-03-06)

### Breaking Change
Expand Down
2 changes: 2 additions & 0 deletions packages/babel-preset-default/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,14 @@ module.exports = function( api ) {
require.resolve( '@wordpress/babel-plugin-import-jsx-pragma' ),
{
scopeVariable: 'createElement',
scopeVariableFrag: 'Fragment',
source: '@wordpress/element',
isDefault: false,
},
],
[ require.resolve( '@babel/plugin-transform-react-jsx' ), {
pragma: 'createElement',
pragmaFrag: 'Fragment',
Copy link
Member

Choose a reason for hiding this comment

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

Re: Abbreviations. I suppose part of the purpose is in inheriting the Babel option by the same name?

https://babeljs.io/docs/en/babel-plugin-transform-react-jsx#pragmafrag

A bit unfortunate naming. I guess I'd rather be consistent than avoid the abbreviation, though.

} ],
require.resolve( '@babel/plugin-proposal-async-generator-functions' ),
maybeGetPluginTransformRuntime(),
Expand Down