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

Make Babel import JSX pragma plugin aware of wp.element.createElement #13809

Merged
merged 6 commits into from
Feb 22, 2019
Merged

Make Babel import JSX pragma plugin aware of wp.element.createElement #13809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5d86909
Test: Add test which verifies wheter JSX pragma detects WP global
gziolo Jan 28, 2019
0d2c51a
Update packages/babel-plugin-import-jsx-pragma/test/index.js
ntwb Feb 5, 2019
14341ac
Skip import when the scope variable is already defined
gziolo Feb 11, 2019
6961bb9
Add failing tests for inner scope variable defined verification
gziolo Feb 20, 2019
2f3986b
Add import statement when there is any undefined scope variable
gziolo Feb 20, 2019
f8793f8
Docs: Add details about changes introduced to Babel plugin
gziolo Feb 20, 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
6 changes: 6 additions & 0 deletions 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.0.0 (Unreleased)

### Breaking Change

- 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)).
Copy link
Member

Choose a reason for hiding this comment

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

  1. I'm having a hard time interpreting this.
  2. Is it really a breaking change? I'm having a hard time thinking how my code could possibly break after the changes here.

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 think you are right. In that sense, all code should work as before. It's definitely some kind of change but I probably went too far and I tried to be overcautious. I will update to New Feature and minor update.

Copy link
Member Author

@gziolo gziolo Feb 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

To be honest, it's hard to decide, you could also consider it as a bug fix :)

Copy link
Member

Choose a reason for hiding this comment

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

I will update to New Feature and minor update.

It was never updated? I think enhancement makes sense, yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, something went wrong during rebase... I also overrode your commit. I will open a follow-up PR. Thanks for catching 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

#14106 opened to fix everything, sorry about that, I still don't know how it has happened ... :(


## 1.1.0 (2018-09-05)

### New Feature
Expand Down
2 changes: 1 addition & 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 @@ -4,7 +4,7 @@ Babel transform plugin for automatically injecting an import to be used as the p

[JSX](https://reactjs.org/docs/jsx-in-depth.html) is merely a syntactic sugar for a function call, typically to `React.createElement` when used with [React](https://reactjs.org/). As such, it requires that the function referenced by this transform be within the scope of the file where the JSX occurs. In a typical React project, this means React must be imported in any file where JSX exists.

**Babel Plugin Import JSX Pragma** automates this process by introducing the necessary import automatically wherever JSX exists, allowing you to use JSX in your code without thinking to ensure the transformed function is within scope.
**Babel Plugin Import JSX Pragma** automates this process by introducing the necessary import automatically wherever JSX exists, allowing you to use JSX in your code without thinking to ensure the transformed function is within scope. It respects existing import statements, as well as scope variable declarations.

## Installation

Expand Down
9 changes: 8 additions & 1 deletion packages/babel-plugin-import-jsx-pragma/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,13 @@ export default function( babel ) {
visitor: {
JSXElement( path, state ) {
state.hasJSX = true;
if ( state.hasUndeclaredScopeVariable ) {
return;
}

const { scopeVariable } = getOptions( state );

state.hasUndeclaredScopeVariable = ! path.scope.hasBinding( scopeVariable );
},
ImportDeclaration( path, state ) {
if ( state.hasImportedScopeVariable ) {
Expand Down Expand Up @@ -71,7 +78,7 @@ export default function( babel ) {
},
Program: {
exit( path, state ) {
if ( ! state.hasJSX || state.hasImportedScopeVariable ) {
if ( ! state.hasJSX || state.hasImportedScopeVariable || ! state.hasUndeclaredScopeVariable ) {
return;
}

Expand Down
40 changes: 38 additions & 2 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 @@ -35,11 +35,18 @@ describe( 'babel-plugin-import-jsx-pragma', () => {
expect( string ).toBe( original );
} );

it( 'does nothing if the scope variable is already defined', () => {
const original = 'const React = require("react");\n\nlet foo = <bar />;';
const string = getTransformedCode( original );

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

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

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

it( 'allows options customization', () => {
Expand All @@ -50,6 +57,35 @@ describe( 'babel-plugin-import-jsx-pragma', () => {
isDefault: false,
} );

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

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}';
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 had some issues with the formatting of the output when changes are applied, @aduth or @nerrad any recommendations on how to better address it?

Are there any reasons we don't use babel-plugin-tester which is highlighted in Babel handbook here: https://github.com/jamiebuilds/babel-handbook/blob/master/translations/en/plugin-handbook.md#babel-plugin-tester?

Copy link
Contributor

Choose a reason for hiding this comment

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

I had some issues with the formatting of the output when changes are applied

Sorry I don't follow, what issues were you having?

Are there any reasons we don't use babel-plugin-tester

I can't give any reasons for why it's not being used, but it looks like it'd be helpful for writing babel plugin tests, so I wouldn't be opposed to it.

Copy link
Member Author

@gziolo gziolo Feb 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I had some issues with the formatting of the output when changes are applied
Sorry I don't follow, what issues were you having?

I had to ensure that I include newline characters and spaces in the original snippet. In particular, concerning was const { createElement } = where Babel wants to force createElement to be in its own file.

const string = getTransformedCode( original, {
scopeVariable: 'createElement',
source: '@wordpress/element',
isDefault: false,
} );

expect( string ).toBe( 'import { createElement } from "@wordpress/element";\n' + original );
Copy link
Contributor

Choose a reason for hiding this comment

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

So does this mean that the plugin is resolving the string to something like this?

import { createElement } from "@wordpress/element";
let foo = <bar />;

function local() {
  const createElement = wp.element.createElement;
}

Do we want that behaviour?

Copy link
Contributor

@nerrad nerrad Feb 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Actually nvm. I understand this is technically okay assuming this kind of script was implemented in an environment which maps the import to the global. It just sounds like its something we shouldn't automatically fix because this kind of thing would likely be found in an environment where there is no aliasing (and thus create a different kind of error (import not found) in the built file for the developer).

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I figured out that I took a very naive approach which won't work in a few cases. Your comment got me thinking what about IIFE which is a very common pattern in the WordPress world. For example:
https://github.com/WordPress/gutenberg/blob/master/packages/e2e-tests/plugins/plugins-api/sidebar.js#L12

I think that the implementation I proposed checks only variable bindings in the very outer scope.

} );

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 string = getTransformedCode( original, {
scopeVariable: 'createElement',
} );

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

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 string = getTransformedCode( original, {
scopeVariable: 'createElement',
} );

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