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

Support React JSX automatic transform #94

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Support React JSX automatic transform #94

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7024076
Add opts.noReactAutoImport
macalinao Nov 30, 2020
011f309
Run transforms before other plugins do
macalinao Dec 1, 2020
50ea24d
fix lint errors
macalinao Dec 1, 2020
aeb8707
Fix tests
macalinao Dec 1, 2020
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
59 changes: 32 additions & 27 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { extname, dirname, parse as parseFilename } from 'path';
import { extname, dirname } from 'path';
import { readFileSync } from 'fs';
import { parse } from '@babel/parser';
import { declare } from '@babel/helper-plugin-utils';
Expand Down Expand Up @@ -115,6 +115,7 @@ export default declare(({
const svgReplacement = buildSvg(opts);
path.replaceWith(svgReplacement);
}

file.get('ensureReact')();
file.set('ensureReact', () => {});
}
Expand All @@ -123,49 +124,53 @@ export default declare(({
return {
visitor: {
Program: {
enter(path, { file, opts, filename }) {
enter(rootPath, state) {
const { opts, filename, file } = state;
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')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

const reactImportDeclaration = t.importDeclaration([
t.importDefaultSpecifier(t.identifier('React')),
], t.stringLiteral('react'));

file.set('ensureReact', () => {
const [newPath] = path.unshiftContainer('body', reactImportDeclaration);
newPath.get('specifiers').forEach((specifier) => { path.scope.registerBinding('module', specifier); });
const [newPath] = rootPath.unshiftContainer('body', reactImportDeclaration);
newPath.get('specifiers').forEach((specifier) => { rootPath.scope.registerBinding('module', specifier); });
});
} else {
file.set('ensureReact', () => {});
}

rootPath.traverse({
CallExpression(path) {
const { node } = path;
const requireArg = node.arguments.length > 0 ? node.arguments[0] : null;
const filePath = t.isStringLiteral(requireArg) ? requireArg.value : null;
if (node.callee.name === 'require' && t.isVariableDeclarator(path.parent) && filePath) {
applyPlugin(path.parent.id, filePath, path.parentPath.parentPath, state);
}
},
ImportDeclaration(path) {
const { node } = path;
if (node.specifiers.length > 0) {
applyPlugin(node.specifiers[0].local, node.source.value, path, state);
}
},
ExportNamedDeclaration(path) {
const { node } = path;
if (node.specifiers.length > 0 && node.specifiers[0].local && node.specifiers[0].local.name === 'default') {
const exportName = node.specifiers[0].exported.name;
applyPlugin(exportName, node.source.value, path, state, true, filename);
}
},
});
},
},
CallExpression(path, state) {
const { node } = path;
const requireArg = node.arguments.length > 0 ? node.arguments[0] : null;
const filePath = t.isStringLiteral(requireArg) ? requireArg.value : null;
if (node.callee.name === 'require' && t.isVariableDeclarator(path.parent) && filePath) {
applyPlugin(path.parent.id, filePath, path.parentPath.parentPath, state);
}
},
ImportDeclaration(path, state) {
const { node } = path;
if (node.specifiers.length > 0) {
applyPlugin(node.specifiers[0].local, node.source.value, path, state);
}
},
ExportNamedDeclaration(path, state) {
const { node } = path;
if (node.specifiers.length > 0 && node.specifiers[0].local && node.specifiers[0].local.name === 'default') {
const exportName = node.specifiers[0].exported.name;
const filename = parseFilename(node.source.value).name;
applyPlugin(exportName, node.source.value, path, state, true, filename);
}
},
},
};
});