Open InvictusMB opened 5 years ago
Thanks for bringing this up @InvictusMB. My inclination is to just throw a hard error with a sensible error message to tell people they can't use macros this way. But then I think why can't they? So instead, do you think that you could make a PR to make this work? I don't want to treat it as a default import though. I want it to work as it normally would (where the namespace specifier is basically an object with all exports on it). What do you think?
I don't have a good answer to this.
I want it to work as it normally would (where the namespace specifier is basically an object with all exports on it).
I agree, however I don't see much syntactic difference between default import and namespace import for a macro user. The only significant thing is that it is illegal to use an imported namespace as a function. Other than that they are functionally equivalent. Especially so, when the majority of npm modules are still bundled as CommonJS modules and in es6 you can import them either way.
My gut feeling tells me that it is better to treat default imports as equal to namespace imports and not complicate 99% of use cases.
Otherwise it seems to be a rabbit hole. Let's say we have this code
import * as foo from 'foo.macro';
foo.bar();
foo["baz"]();
I assume, you want it to be handled as equivalent to
import {bar, baz} from 'foo.macro';
bar();
baz();
The algorithm could be:
MemberExpression
or JSXMemberExpression
property
pathproperty
is Identifier
save name
as a nameproperty
is Literal
save value
as a nameIdentifier
or JSXIdentifier
then fallback to default
name or throw (???)On the macro side this will also complicate things.
With named import bar
will always be an Identifier
or JSXIdentifier
.
With namespaced import we will get a reference named bar
but the path can either be MemberExpression
or JSXMemberExpression
if we passed parent path.
Otherwise it can be Identifier
or JSXIdentifier
and the macro has to check if it is under MemberExpression
or JSXMemberExpression
and use parent path instead of provided path for replacement.
I'm afraid this will be confusing both ways.
I'm not 100% sure this extra complexity is worth the outcome.
P.S. The test suite fails on my windows machine. Does it have any windows specific reasons to fail?
λ npm test
> babel-plugin-macros@0.0.0-semantically-released test C:\Projects\babel-plugin-macros
> kcd-scripts test
PASS src/__tests__/create-macros.js
FAIL src/__tests__/index.js
● Test suite failed to run
Cannot find module 'babel-plugin-macros-test-fake/macro' from 'C:\Projects\babel-plugin-macros\src\__tests__'
at Function.module.exports [as sync] (node_modules/resolve/lib/sync.js:69:15)
at nodeResolvePath (node_modules/babel-plugin-macros/dist/index.js:49:18)
at applyMacros (node_modules/babel-plugin-macros/dist/index.js:178:23)
at VariableDeclaration.path.get.filter.forEach.child (node_modules/babel-plugin-macros/dist/index.js:126:30)
at Array.forEach (<anonymous>)
at VariableDeclaration (node_modules/babel-plugin-macros/dist/index.js:116:55)
at NodePath._call (node_modules/@babel/traverse/lib/path/context.js:53:20)
at NodePath.call (node_modules/@babel/traverse/lib/path/context.js:40:17)
at NodePath.visit (node_modules/@babel/traverse/lib/path/context.js:88:12)
at TraversalContext.visitQueue (node_modules/@babel/traverse/lib/context.js:118:16)
I'm also not convinced it's worth the work.
Tests should definitely work on windows, but I don't have a windows machine to test it on. PRs welcome for that!
I have a PR on the way for this. So the suggested plan is: 1) For now namespace imports will be treated the same way as default imports. There is no reason not to as the only semantic difference is that a namespace is not callable. But it is not the job of this plugin to enforce that. TypeScript and linters can handle the semantics for us.
2) If the demand for a separate handling of namespaces appears it can be later introduced together with a config switch. I believe in this case the plugin would have to export SymbolNamespace
symbol for macros to identify namespace imports as we cannot simply use namespace
since it is not a reserved word.
SymbolNamespace
could be used here
https://github.com/kentcdodds/babel-plugin-macros/blob/2d57c60adadd8904db9624614faffc34a251c40d/src/index.js#L97-L100
3) If there is a need to track foo.bar
usages to make them equivalent to import {bar}
then a separate helper can be released. It will probably require some additional config from macro creators for handling of corner cases. But for the scope of this plugin it is a bit too much of complexity. The complexity comes from the fact that imported bar
is a standalone reference but namespaced foo.bar
is an expression and needs a different treatment inside the macro itself. One way to workaround that could be to have 2 runs where the first will transform import * as foo
into import {bar as <uniqueId>}
based on references further in the program and then the second run will do the regular handling of import specifiers.
You know what, I'd really rather wait until someone's really asking for this before making a bunch of changes 😬
I agree. I want the plugin not crashing though. And I believe we have enough arguments to fix this in the least effort way.
@kentcdodds Was this fixed?
No, but nobody's actively working on it or asking for it. Sorry, I should have commented about that when closing.
You know what, I'll go ahead and work on this myself.
babel-plugin-macros
version: 2.5.1Relevant code or config
What happened: Compiler fails with the following message
Problem description:
ImportNamespaceSpecifier
node is not handled properly here and the error message is not helpful. Offending code: https://github.com/kentcdodds/babel-plugin-macros/blob/e0ebca780856be76cbe1c5841d09a922fdde533b/src/index.js#L78-L80Suggested solution:
ImportNamespaceSpecifier
should either be handled asdefault
case or as a separatenamespace
case. Or maybe both variants are valid and there should be a config option akin toallowSyntheticDefaultImports
in TypeScriptQuick fix: