Framework: Add package: @wordpress/babel-plugin-import-jsx-pragma #7493

aduth · 2018-06-22T18:51:20Z

Fixes #6956
Related: #742 (specifically #742 (comment) )

This pull request seeks to introduce a new package, @wordpress/babel-plugin-import-jsx-pragma, which seeks to serve as a compromise between competing objectives of developer experience (not needing explicit import to use JSX) and eliminating reliance on globals (previously the pragma was configured to a wp.element.createElement global). The transform effectively guarantees that the required import is made to be in scope wherever JSX is used, and updates our configured pragma from wp.element.createElement to simply createElement.

See included README.md
See demonstration

// Before:
const MyComponent = () => <div />;

// After:
import React from "react";
const MyComponent = () => <div />;

It resolves #6956 where end-to-end tests fail due to a conflict between the change detection tests reloading the page and core logic to unset the window.wp global on page unload, where previously failures could occur when a render happened after the unsetting (as it would attempt to unsuccessfully call the missing wp.element.createElement).

Testing instructions:

Verify that the editor loads without issue.

Ensure unit tests pass.

aduth · 2018-06-22T19:03:49Z

The test failure surfaces an interesting problem, which is that in order to build packages, we now need this packaged Babel plugin (chicken and egg problem). We could either (a) publish the package once manually, then reference the published version or (b) update the build process for packages to first build the required babel-plugin-import-jsx-pragma standalone.

tofumatt · 2018-06-22T19:45:06Z

update the build process for packages to first build the required babel-plugin-import-jsx-pragma standalone

Option "B" seems like the better one to me.

tofumatt

I think this is nice and I just have some thoughts on the docs.

tofumatt · 2018-06-22T19:47:27Z

packages/babel-plugin-import-jsx-pragma/README.md

+It's assumed that you're also using [@babel/transform-react-jsx](https://babeljs.io/docs/en/babel-plugin-transform-react-jsx/).
+Otherwise, you may encounter errors when encountering JSX tokens.
+
+```


You should mark this as js so it'll syntax-highlight nicely on GitHub, eg:

```js // .babelrc.js module.exports = { plugins: [ '@wordpress/babel-plugin-import-jsx-pragma', '@babel/transform-react-jsx', ], }; ```

tofumatt · 2018-06-22T19:50:56Z

packages/babel-plugin-import-jsx-pragma/README.md

+Refer to the [Babel Plugins documentation](http://babeljs.io/docs/en/plugins)
+if you don't yet have experience working with Babel plugins.
+
+Include `@wordpress/babel-plugin-import-jsx-pragma` as a plugin in your Babel


This could be reworded to be instructive and less vague, as for any standard setup there'll be errors, and if the setup is non-standard we can assume developer familiarity. How about just:

Include `@wordpress/babel-plugin-import-jsx-pragma` (and [@babel/transform-react-jsx](https://babeljs.io/docs/en/babel-plugin-transform-react-jsx/)) as plugins in your Babel configuration. If you don't include both you will get errors with encountering JSX tokens.

tofumatt · 2018-06-22T19:51:39Z

packages/babel-plugin-import-jsx-pragma/README.md

+
+## Options
+
+As the `@babel/transform-react-jsx` plugin offers options to customize the


Do we usually newline our markdown? If you don't it's nicer to diff/edit, I think doing this is annoying when making changes.

Do we usually newline our markdown? If you don't it's nicer to diff/edit, I think doing this is annoying when making changes.

We have at some points. This is my first go at it. In retrospect, I agree that I don't like it, and we should probably discourage it. Updated in aad1bed.

tofumatt · 2018-06-22T19:53:31Z

packages/babel-plugin-import-jsx-pragma/src/index.js

+				const { scopeVariable, isDefault } = getOptions( state.opts );
+
+				// The module source isn't verified, since if at least the
+				// required variable is within scope, its assumed to be


its -> it's

tofumatt · 2018-06-22T19:54:52Z

packages/babel-plugin-import-jsx-pragma/src/index.js

+
+				const { scopeVariable, isDefault } = getOptions( state.opts );
+
+				// The module source isn't verified, since if at least the


I think I get this comment but it's a bit hard to parse, especially because "since" could be "because" to make it read more logically.

I don't have ideas for copyediting right now but could this be explained another way? Or maybe broken up into a few sentences so it's easier to parse?

tofumatt · 2018-06-22T19:54:57Z

packages/babel-plugin-import-jsx-pragma/src/index.js

+				// required variable is within scope, its assumed to be
+				// compatible with the targeted transform, and otherwise would
+				// conflict as a duplicate import if introduced separately.
+


This newline is weird to me.

This newline is weird to me.

For context, there was previously a condition here which checked that the import source matched what we were expecting, but specifically I ran into issues with @wordpress/element where we import createElement from React proper:

gutenberg/packages/element/src/index.js

Lines 4 to 15 in 04c0ea2

import {

createElement,

createContext,

createRef,

forwardRef,

Component,

cloneElement,

Children,

Fragment,

isValidElement,

StrictMode,

} from 'react';

I will update the comment and shift it down a line to describe the behavior as part of what it is we're actually testing now.

aduth · 2018-06-26T16:12:39Z

Thanks for the review @tofumatt . I've pushed a few updates, including one to prebuild the @wordpress/babel-plugin-import-jsx-pragma separate from other packages.

youknowriad · 2018-06-27T12:55:25Z

bin/packages/get-babel-config.js

+		source: '@wordpress/element',
+		isDefault: false,
+	} ] );
+}


Do you think we should include this in the babel preset package?

Yes 😄 It's been an awkward struggle thus far with the preset. I imagine it will be made easier once it's part of Gutenberg proper.

youknowriad · 2018-06-27T12:57:45Z

bin/packages/get-babel-config.js

@@ -19,6 +19,14 @@ const plugins = map( babelDefaultConfig.plugins, ( plugin ) => {
 	return plugin;
 } );

+if ( process.env.TRANSFORM_JSX_PRAGMA ) {


Also not sure why we need all these env variables (this one and the include/exclude ones)
Can't we just always use this plugin? What's the issue if always include it?

See #7493 (comment)

I see so I guess this would solve the issue right? #7556

youknowriad · 2018-06-27T13:00:51Z

packages/babel-plugin-import-jsx-pragma/src/index.js

+	return {
+		visitor: {
+			JSXElement() {
+				hasJSX = true;


I read somewhere in babel docs that it's not great to rely on state variables like that and that it's probably better to rely on the "state" object passed to the visitors because of the way these visitors can be called in random orders.

I'm not a babel-plugin developer so I can't argue if it's legit here or not, just wanted to mention it.

Based on the handbook description of visiting behavior, and particularly with its evaluation on the Program.exit pass, I don't know that it should be a concern.

https://github.com/jamiebuilds/babel-handbook/blob/master/translations/en/plugin-handbook.md#visitors

That said, I'll take a look to see if it's easy enough to switch to using state.

Updated in 67653d0. Makes things simpler overall too.

youknowriad

LGTM globally, the issue I have is these global variables, it makes it hard to follow. When should I use these globals and when not. I think it's better if it's handled automatically somehow.

aduth · 2018-06-27T13:07:47Z

the issue I have is these global variables, it makes it hard to follow. When should I use these globals and when not. I think it's better if it's handled automatically somehow.

Which globals are you referring to? The ultimate goal is that we don't rely on any globals. JSX is more of a special case in that it's technically magic that it's importing the createElement, but then again, there's nothing explicit about <Foo /> being converted to a pragma in the first place.

youknowriad · 2018-06-27T13:08:31Z

Sorry I meant the environment variables

aduth · 2018-06-27T13:10:01Z

I don't know that we should need to use the environment variables for any other package. It's more exclusively affecting Babel plugins where we expect to use the (built) plugin in the building of other packages. The other option is to rewrite the plugin so it doesn't require a build step (ES5).

youknowriad · 2018-06-27T13:14:23Z

Ok I see, I think I'm fine with what we have. We can see if it's a lot of struggle over time and if we'd have to differentiate between "node/babel packages" and browser ones.

Framework: Add package: @wordpress/babel-plugin-import-jsx-pragma

c846b72

aduth added Framework Issues related to broader framework topics, especially as it relates to javascript npm Packages Related to npm packages labels Jun 22, 2018

tofumatt reviewed Jun 22, 2018

View reviewed changes

aduth added 2 commits June 26, 2018 12:05

Build: Prebuild babel-plugin-import-jsx-pragma package

09feb72

Build: Improve import-jsx-pragma docs per review feedback

aad1bed

Build: Consolidate directory testing into filterPackages

980d38b

aduth mentioned this pull request Jun 27, 2018

Intermittent e2e test failures #6956

Closed

youknowriad reviewed Jun 27, 2018

View reviewed changes

youknowriad approved these changes Jun 27, 2018

View reviewed changes

Build Tools: Leverage plugin state in place of scope variables

67653d0

aduth merged commit 6a52599 into master Jun 27, 2018

aduth deleted the fix/intermittent-e2e-failure-in-flight branch June 27, 2018 14:23

This was referenced Jun 28, 2018

Use withSafeTimeout in NUX tips #7544

Merged

Build: Use cross-env for plugin build scripts #7615

Merged

Build: Invert JSX pragma application condition #7701

Merged

aduth mentioned this pull request Jul 6, 2018

NUX: Avoid popover refresh on tip mount #7748

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Framework: Add package: @wordpress/babel-plugin-import-jsx-pragma #7493

Framework: Add package: @wordpress/babel-plugin-import-jsx-pragma #7493

aduth commented Jun 22, 2018

aduth commented Jun 22, 2018

tofumatt commented Jun 22, 2018

tofumatt left a comment

tofumatt Jun 22, 2018

tofumatt Jun 22, 2018

tofumatt Jun 22, 2018

aduth Jun 26, 2018

tofumatt Jun 22, 2018

tofumatt Jun 22, 2018

tofumatt Jun 22, 2018

aduth Jun 26, 2018

aduth commented Jun 26, 2018

youknowriad Jun 27, 2018

aduth Jun 27, 2018

youknowriad Jun 27, 2018

aduth Jun 27, 2018

youknowriad Jun 27, 2018

youknowriad Jun 27, 2018

aduth Jun 27, 2018

aduth Jun 27, 2018

youknowriad left a comment

aduth commented Jun 27, 2018 •

edited

Loading

youknowriad commented Jun 27, 2018

aduth commented Jun 27, 2018

youknowriad commented Jun 27, 2018


		## Options

		As the `@babel/transform-react-jsx` plugin offers options to customize the


		const { scopeVariable, isDefault } = getOptions( state.opts );

		// The module source isn't verified, since if at least the

	import {
	createElement,
	createContext,
	createRef,
	forwardRef,
	Component,
	cloneElement,
	Children,
	Fragment,
	isValidElement,
	StrictMode,
	} from 'react';

Framework: Add package: @wordpress/babel-plugin-import-jsx-pragma #7493

Framework: Add package: @wordpress/babel-plugin-import-jsx-pragma #7493

Conversation

aduth commented Jun 22, 2018

aduth commented Jun 22, 2018

tofumatt commented Jun 22, 2018

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Jun 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

aduth commented Jun 27, 2018 • edited Loading

youknowriad commented Jun 27, 2018

aduth commented Jun 27, 2018

youknowriad commented Jun 27, 2018

aduth commented Jun 27, 2018 •

edited

Loading