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.

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

Run jscodeshift with --no-babel #266

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chancancode
Copy link

Opening this for discussion. Context:

By default, jscodeshift use babel to transform the transform files (i.e. the "plugins") themselves before running them in Node. The motivation of this seems to be to allow using more modern features in the transform plugins.

In the "old days" this may be necessary/desirable, but with modern Node versions having pretty good support for modern features/syntaxes, this benefit seems margin/dubious, and it can sometimes cause problem.

One example is that we tried to use @embroider/core in our transform, which depends on a fairly old package called sourcemap-validator. The source code of the package is NOT written with strict mode in mind, but the default settings for jscodeshift's babel transform blindly adds the "use strict"; declaration on top of each file, including dependencies in node_modules.

This ultimately causes a hard error when running the codemod, in this case a ReferenceError: createBuffer is not defined when loading the file. Coincidentally the jscodeshift set up of running things in workers and babel overriding require() in Node also makes it harder to discover the root cause of the problem.

It's unclear what can be done about this – I tried adding an empty .babelrc on the transform plugin side but that doesn't seem to be picked up. I am not sure how we are supposed to customize the babel options used for this purpose (which also further diminishes its utility).

Ultimately, given the utility of this seems dubious on modern node, most transforms probably do not take advantage of this and most authors and consumers probably aren't aware of this little-known feature on jscodeshift, it's easier to just disable that which would probably make things run a little faster.

It's also unclear if this ever worked "correctly" given that the template transforms don't get the same babel treatment.


As an alternative, we can also make it possible to pass this option through to jscodeshift from runTransform.

However, this would require some refactoring/rethinking of parseTransformArgs to make it possible, i.e. it's not as simple as adding it to the list, which was the first thing I tried.

The main problem is yargs parses --no-babel into { babel: false } and the current code does not handle that well. (I think the current code actually already has problems with things like --verbose.)

If, for some reason, it's desirable to keep the babel option on the jscodeshift side working, I can look into doing the work, but given what I said above I personally don't see a good reason to do that.

By default, jscodeshift use babel to transform the transform files
(i.e. the "plugins") themselves before running them in Node. The
motivation of this seems to be to allow using more modern features in the
transform plugins.

In the "old days" this may be necessary/desirable, but with modern Node
versions having pretty good support for modern features/syntaxes, this
benefit seems margin/dubious, and it can sometimes cause problem.

One example is that we tried to use `@embroider/core` in our transform,
which depends on a fairly old package called `sourcemap-validator`. The
source code of the package is NOT written with strict mode in mind, but
the default settings for jscodeshift's babel transform blindly adds the
`"use strict";` declaration on top of each file, including dependencies
in node_modules.

This ultimately causes a hard error when running the codemod, in this 
case a `ReferenceError: createBuffer is not defined` when loading the
file. Coincidentally the `jscodeshift` set up of running things in
workers and babel overriding `require()` in Node also makes it harder
to discover the root cause of the problem.

It's unclear what can be done about this – I tried adding an empty
`.babelrc` on the transform plugin side but that doesn't seem to be
picked up. I am not sure how we are supposed to customize the babel
options used for this purpose (which also further diminishes its
utility).

Ultimately, given the utility of this seems dubious on modern node,
most transforms probably do not take advantage of this and most
authors and consumers probably aren't aware of this little-known
feature on `jscodeshift`, it's easier to just disable that which 
would probably make things run a little faster.

It's also unclear if this ever worked "correctly" given that the
template transforms don't get the same babel treatment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant