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

Babel plugin JSX: Implement Fragment handling #15120

Merged
merged 7 commits into from
Apr 30, 2019
Merged
Changes from 1 commit
Commits
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
Next Next commit
Implement Fragment handling
sirreal committed Apr 30, 2019

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit 6f5fa88865a44cfd219665939e65a9db7da3aeb3
90 changes: 63 additions & 27 deletions packages/babel-plugin-import-jsx-pragma/index.js
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

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,
};
@@ -32,47 +35,80 @@ module.exports = function( babel ) {
function getOptions( state ) {
if ( ! state._options ) {
state._options = Object.assign( {}, DEFAULT_OPTIONS, state.opts );
if ( state._options.isDefault && state._options.scopeVariableFrag ) {
// eslint-disable-next-line no-console
console.warn( 'scopeVariableFrag is only available when isDefault is false' );
sirreal marked this conversation as resolved.
Show resolved Hide resolved
state._options.scopeVariableFrag = null;
}
}

return state._options;
}

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 );
},
'JSXElement|JSXFragment'( path, state ) {
if ( state.hasUndeclaredScopeVariableFrag ) {
return;
}

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

if (
path.type === 'JSXFragment' ||
( path.type === 'JSXElement' && path.node.openingElement.name.name === 'Fragment' )
) {
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 );
}
},
},
},
99 changes: 84 additions & 15 deletions packages/babel-plugin-import-jsx-pragma/test/index.js
Original file line number Diff line number Diff line change
@@ -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 );
@@ -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, {
@@ -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',
@@ -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( 'adds 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, Fragment } 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;
}