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

fix: handle namespace imports gracefully #122

Closed
wants to merge 1 commit into from

Conversation

InvictusMB
Copy link
Contributor

What:

Fix #111

How:

Handle namespace imports as default imports.

Why

Historically default imports are there in es6 only because of CommonJS interop compatibility and in the context of such an interop (or more broadly any non es module interop) Babel itself populates default property in the same way for both namespace and default imports.

function _interopRequireDefault(obj) {
  return obj && obj.__esModule ? obj : { default: obj };
}

function _interopRequireWildcard(obj) {
  if (obj && obj.__esModule) {
    return obj;
  } else {
    var newObj = {};
    /* ... copy props ... */
    newObj["default"] = obj;
    return newObj;
  }
}

If we accept that importing from a macro is not exactly the same thing as consuming an es module and follow a formal logic of Babel it would be justifiable for babel-plugin-macros to coerce a namespace import to a default import.

@InvictusMB
Copy link
Contributor Author

It seems one of the dependencies broke the tests. They work perfectly with my 20 days old package-lock.json but fail after a fresh install. Same happens to me on master.

@kentcdodds
Copy link
Owner

Just to be clear:

import * as foo from 'foo.macro'

foo.baz('hi')

// would basically be the same as

import baz from 'foo.macro/baz'

baz('hi')

So as the macro author I would need a baz module if I want to support namespaces?

If that's the case then we definitely cannot do this... The only way I'll do anything about namespaces is if:

import * as foo from 'foo.macro'
foo.baz('hi')
// is the same as
import {baz} from 'foo.macro'
baz('hi')

@InvictusMB
Copy link
Contributor Author

No, as the macro author you just don't distinguish between import * as foo and import foo. No extra effort by macro authors is needed to support namespace imports.

It is as if

import * as foo from 'foo.macro'

foo.baz('hi')

// before running a macro would become
import * as foo from 'foo.macro'

foo.default.baz('hi')

The added tests contain a simplified version of a macro I already use. The original version proxies imports from Material UI so that I could use a wildcard import without compromising the tree shaking. Except that I can not fully replicate the semantics of import * as MUI from '@material-ui/core' with a macro yet because import * as MUI from 'my-mui-proxy.macro' crashes the plugin.

Lastly, if a macro author wishes to make

import * as foo from 'foo.macro'
foo.baz('hi')
// the same as
import {baz} from 'foo.macro'
baz('hi')

he can still do so himself. In fact most of the code to implement that is already present in my test macro. It just needs to be generalized and extracted as a helper.

@kentcdodds
Copy link
Owner

Oh, I see. I really want this:

import * as foo from 'foo.macro'
foo.baz('hi')
// the same as
import {baz} from 'foo.macro'
baz('hi')

Without the author having to do anything fancy (like having to apply the baz behavior to both references.baz and references.default.baz).

@InvictusMB
Copy link
Contributor Author

It is far from trivial to make this transparent to a macro as I detailed out in the #111 thread.
The two cases differ from AST perspective.
foo.baz reference is an expression and baz reference alone is an identifier. Either a macro will now have to handle that difference in context (breaking change) or a plugin needs to preprocess the AST and inline foo.baz as baz possibly running into naming collisions (rabbit hole of complexity).

Also if we managed to solve the above mentioned issues what would happen here?

import * as foo from 'foo.macro'
const bar = foo
// or
console.log(foo)

It is a reference but not to a named import and now not a default either. How do we pass it to a macro?
We would again need something like SymbolNamespace for a macro to be able to access this reference within references object without a risk of naming collisions.

@InvictusMB
Copy link
Contributor Author

I could also argue that if we transparently handle cases of import * as foo we should do the same for import foo. Or otherwise it will confuse a considerable number of macro users.

Articles such as this show that the difference between import * as foo and import foo is not obvious.

@kentcdodds
Copy link
Owner

the difference between import * as foo and import foo is not obvious.

The difference may not be obvious, but I'm committed to try to follow the spec as closely as possible and not drag CommonJS issues along with us into the future.

@kentcdodds kentcdodds closed this Nov 15, 2019
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.

Namespace import crashes the plugin
2 participants