Support React JSX automatic transform #94

macalinao · 2020-12-01T05:50:17Z

The React JSX transform detects JSX by seeing if it's already in the file, but it won't know about transformed JSX if the node visitors were never hit.

https://github.com/babel/babel/blob/02fc9e835ceac71eb4de7dac6137fd0489176c29/packages/babel-helper-builder-react-jsx-experimental/src/index.js#L328

This new functionality can be enabled with noReactAutoImport: true in .babelrc.

Fixes #91.

macalinao · 2020-12-01T05:53:26Z

You can test this today using yarn add -D @simplyianm/babel-plugin-inline-react-svg.

ljharb · 2020-12-01T06:41:30Z

src/index.js

          if (typeof filename === 'string' && typeof opts.filename !== 'undefined') {
            throw new TypeError('the "filename" option may only be provided when transforming code');
          }
          if (typeof filename === 'undefined' && typeof opts.filename !== 'string') {
            throw new TypeError('the "filename" option is required when transforming code');
          }
-          if (!path.scope.hasBinding('React')) {
+
+          if (!opts.noReactAutoImport && !rootPath.scope.hasBinding('React')) {


Can you help me understand why this line alone is not sufficient to address the issue?

The applyPlugin calls need to be made before subsequent plugins (specifically the next/babel preset) are called. The previous method called the JSX transform's visitor.Program.enter before doing those, causing the JSX transform to incorrectly think that the file didn't contain any JSX.

Transform ordering is something that is usually dealt with in the babel config. This does mean, if you need a plugin to run before a preset, that you have to create your own preset that uses the plugin, and then use that preset in your own config instead of that plugin.

If you do that, does that obviate the need for all the larger changes?

Yes, but that would require a lot of work for anyone that uses presets. I think it makes more sense to ensure the plugin runs before everything else does. I believe that other plugins in the plugins array are also run before applyPlugin.

That's the nature of babel, though - and not something individual transforms should be solving.

I'd assume that those using it need to ensure that it runs last.

The thing is, it can never run last if the visitor isn't written this way. visitor.Program.enter of all plugins is run before we ever hit an applyPlugin, and the React JSX transform's visitor.Program.enter determines whether or not it will operate on the file.

aha, ok thanks for clarifying - iow, we must write it this way explicitly because of the way the new jsx transform is authored.

Might this cause an ordering change that could break existing users?

It's possible, since subsequent plugins will now be seeing the SVGs as JSX instead of as imports

Could we make the ordering changes pivot based on the noReactAutoImport setting?

bearcott · 2020-12-01T19:08:46Z

wow! this is so cool I need this.

diegopelusa · 2020-12-23T09:11:45Z

Hi!
Can this update be reviewed?
Sorry, that ask for it here. But it seems it is only pending for a reviewer for 2 weeks.
I just don't understand it properly. But it seems that it would resolve the problem (#91), which I am having.
Many thanks!

ljharb · 2020-12-23T16:09:25Z

@diegopelusa its been reviewed, and is waiting for a response from the PR author.

diegopelusa · 2020-12-28T09:05:43Z

Many thanks @ljharb !!!
Let's hope that @macalinao sees these comments and respond to the review comments.

macalinao · 2020-12-28T20:26:51Z

Many thanks @ljharb !!!
Let's hope that @macalinao sees these comments and respond to the review comments.

Hey sorry, I've been a bit busy recently, but if you want my changes you can install @simplyianm/babel-plugin-inline-react-svg.

diegopelusa · 2020-12-29T08:18:44Z

Many thanks @macalinao!!!!
I would use your fork of the plugin, but I think the best way to get a stable product would be to merge into the one in airbnb.
Hmm!!
(If I could help you merge the changes, I would do it. But I have too little idea about all this software)
Anyway, many thanks, and hopefully you get some time to close this PR.

diegopelusa · 2021-01-08T10:37:00Z

Any chance to merge this code?
Only @macalinao can do it?
I am currently avoiding to have React17 in my NextJS10 project, because there are errors with this plugin.
Is there anything I can do, but just waiting?
Or is now the fork @simplyianm/babel-plugin-inline-react-svg the one to be used for this plugin.
Sorry, that I ask to impatiently. But I don´t know what to do next in this issue.
Many thanks to everyone helping in this project!

kripod · 2021-01-08T13:47:46Z

Any chance to merge this code?
Only @macalinao can do it?
I am currently avoiding to have React17 in my NextJS10 project, because there are errors with this plugin.
Is there anything I can do, but just waiting?
Or is now the fork @simplyianm/babel-plugin-inline-react-svg the one to be used for this plugin.
Sorry, that I ask to impatiently. But I don´t know what to do next in this issue.
Many thanks to everyone helping in this project!

You may upgrade to Next.js 10 and React 17 while using the classic React runtime via .babelrc:

{
	"presets": [["next/babel", { "preset-react": { "runtime": "classic" } }]],
	"plugins": ["babel-plugin-inline-react-svg"]
}

Please be patient towards the maintainers and the PR’s author, especially in the rough times we are living in nowadays.

diegopelusa · 2021-01-10T10:43:19Z

Hi @kripod !
Many thanks for the answer. I will try your proposed solution.
I am sorry if anyone thinks I am impatient of pushing anyone to do something that is not appropriate in these days.
I just wanted to know how to solve the current situation with the problem of React17, Next10 and this plugin.
Sorry again if I asked in the wrong place or with bad words.
That was not my intention!

ljharb · 2021-01-10T14:58:39Z

This is waiting on the OP to make the changes asked about here, or, for someone else to comment here (not a new PR, please) with a link to those updated changes, so i can pull them in to this PR.

robert-hurst-cmd · 2021-01-22T06:34:14Z

@macalinao Any chance you could follow this one up? Would love to see your work get merged. :)

macalinao added 2 commits November 30, 2020 15:52

Add opts.noReactAutoImport

7024076

Run transforms before other plugins do

011f309

fix lint errors

50ea24d

macalinao mentioned this pull request Dec 1, 2020

'undefined is not a function' when using the new JSX transform #91

Open

Fix tests

aeb8707

ljharb reviewed Dec 1, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support React JSX automatic transform #94

Support React JSX automatic transform #94

macalinao commented Dec 1, 2020 •

edited

Loading

macalinao commented Dec 1, 2020

ljharb Dec 1, 2020

macalinao Dec 1, 2020

ljharb Dec 3, 2020

macalinao Dec 7, 2020

ljharb Dec 7, 2020

ljharb Dec 7, 2020

macalinao Dec 8, 2020

ljharb Dec 8, 2020

macalinao Dec 8, 2020

ljharb Dec 8, 2020

bearcott commented Dec 1, 2020

diegopelusa commented Dec 23, 2020

ljharb commented Dec 23, 2020

diegopelusa commented Dec 28, 2020

macalinao commented Dec 28, 2020

diegopelusa commented Dec 29, 2020

diegopelusa commented Jan 8, 2021

kripod commented Jan 8, 2021 •

edited

Loading

diegopelusa commented Jan 10, 2021

ljharb commented Jan 10, 2021

robert-hurst-cmd commented Jan 22, 2021

This comment has been minimized.

Support React JSX automatic transform #94

Are you sure you want to change the base?

Support React JSX automatic transform #94

Conversation

macalinao commented Dec 1, 2020 • edited Loading

macalinao commented Dec 1, 2020

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

Choose a reason for hiding this comment

bearcott commented Dec 1, 2020

diegopelusa commented Dec 23, 2020

ljharb commented Dec 23, 2020

diegopelusa commented Dec 28, 2020

macalinao commented Dec 28, 2020

diegopelusa commented Dec 29, 2020

diegopelusa commented Jan 8, 2021

kripod commented Jan 8, 2021 • edited Loading

diegopelusa commented Jan 10, 2021

ljharb commented Jan 10, 2021

robert-hurst-cmd commented Jan 22, 2021

This comment has been minimized.

macalinao commented Dec 1, 2020 •

edited

Loading

kripod commented Jan 8, 2021 •

edited

Loading