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

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

Merged
merged 5 commits into from Jun 27, 2018

Conversation

Projects
None yet
3 participants
@aduth
Member

aduth commented Jun 22, 2018

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

This comment has been minimized.

Member

aduth commented Jun 22, 2018

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

This comment has been minimized.

Member

tofumatt commented Jun 22, 2018

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.

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.
```

This comment has been minimized.

@tofumatt

tofumatt Jun 22, 2018

Member

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',
	],
};
```
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 comment has been minimized.

@tofumatt

tofumatt Jun 22, 2018

Member

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.
## Options
As the `@babel/transform-react-jsx` plugin offers options to customize the

This comment has been minimized.

@tofumatt

tofumatt Jun 22, 2018

Member

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.

This comment has been minimized.

@aduth

aduth Jun 26, 2018

Member

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.

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

This comment has been minimized.

@tofumatt

tofumatt Jun 22, 2018

Member

its -> it's

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

This comment has been minimized.

@tofumatt

tofumatt Jun 22, 2018

Member

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?

// 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 comment has been minimized.

@tofumatt

tofumatt Jun 22, 2018

Member

This newline is weird to me.

This comment has been minimized.

@aduth

aduth Jun 26, 2018

Member

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:

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

This comment has been minimized.

Member

aduth commented Jun 26, 2018

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.

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

This comment has been minimized.

@youknowriad

youknowriad Jun 27, 2018

Contributor

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

This comment has been minimized.

@aduth

aduth Jun 27, 2018

Member

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.

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

This comment has been minimized.

@youknowriad

youknowriad Jun 27, 2018

Contributor

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?

This comment has been minimized.

@aduth

This comment has been minimized.

@youknowriad

youknowriad Jun 27, 2018

Contributor

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

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

This comment has been minimized.

@youknowriad

youknowriad Jun 27, 2018

Contributor

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.

This comment has been minimized.

@aduth

aduth Jun 27, 2018

Member

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.

This comment has been minimized.

@aduth

aduth Jun 27, 2018

Member

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

This comment has been minimized.

Member

aduth commented Jun 27, 2018

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

This comment has been minimized.

Contributor

youknowriad commented Jun 27, 2018

Sorry I meant the environment variables

@aduth

This comment has been minimized.

Member

aduth commented Jun 27, 2018

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

This comment has been minimized.

Contributor

youknowriad commented Jun 27, 2018

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.

@aduth aduth merged commit 6a52599 into master Jun 27, 2018

2 checks passed

codecov/project 47.01% (+0.21%) compared to b16a8b2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the fix/intermittent-e2e-failure-in-flight branch Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment