-
Notifications
You must be signed in to change notification settings - Fork 204
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
Simplify path replacing #136
Conversation
Codecov Report
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @fatfisz!
visitor: { | ||
Program: { | ||
exit(programPath, state) { | ||
programPath.traverse(importVisitors, state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How good/bad this is?
Is it the same in term of performance as what was done before? I remember reading that if we can do something without traversing
, it's usually best
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought of other solutions, too, but each one of them fell short:
- caching the paths won't help with all the edge cases: let's say that path A -> path B, so path B is marked as untouchable - what about path B appearing in some other place? This could result in the loss of determinism (result would depend on the order of the paresd files and would be bad anyway)
- "marking" the paths in any way is not viable - I tried wrapping the path in the
String
object, which didn't play with the other plugin; also the node may get replaced by other plugins which will result in loss of the metadata (babel-plugin-transform-es2015-modules-commonjs
is takingvalue
and putting it into a newStringLiteral
node)
FWIW babel-plugin-transform-es2015-modules-commonjs
is also doing something similar to traverse
on Program exit (it doesn't have to do the full traverse, because import
is only allowed at the top level).
src/transformers/require.js
Outdated
nodePath.replaceWith(t.callExpression( | ||
calleePath.node, [t.stringLiteral(modulePath)], | ||
)); | ||
nodePath.get('arguments.0').replaceWith(t.stringLiteral(modulePath)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, we can use moduleArg
instead of nodePath.get('arguments.0')
src/transformers/jest.js
Outdated
nodePath.replaceWith(t.callExpression( | ||
calleePath.node, newArgs, | ||
)); | ||
nodePath.get('arguments.0').replaceWith(t.stringLiteral(modulePath)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! We can even do better by replacing nodePath.get('arguments.0')
with moduleArg
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't know that elements of the arguments
path were also paths. Will fix that.
src/transformers/systemImport.js
Outdated
nodePath.replaceWith(t.callExpression( | ||
calleePath.node, [t.stringLiteral(modulePath)], | ||
)); | ||
nodePath.get('arguments.0').replaceWith(t.stringLiteral(modulePath)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for nodePath.get('arguments.0')
@tleunen Ping ;) Btw. is there a chance you'll be releasing a new version with the fixes anytime soon? |
I'm good with these changes, no worries :) |
This branch consists of a few changes, so maybe it's better to split it into a few smaller ones.
The changes are:
The last part changes the behavior of the plugin, so at least a minor version bump would be needed.