Skip to content

Instantly share code, notes, and snippets.

@Rich-Harris
Created May 18, 2017 16:40
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save Rich-Harris/24a8ddd3947150aa8c15a9367faf1d62 to your computer and use it in GitHub Desktop.
Save Rich-Harris/24a8ddd3947150aa8c15a9367faf1d62 to your computer and use it in GitHub Desktop.
why deopting on `import *` is bad

Most people treat these is interchangeable:

// named import
import { bar } from './foo.js';
bar();

// namespace import
import * as foo from './foo.js';
foo.bar();

Generally, the point here is that foo is a collection of functions (or other values) — it's not supposed to be thought of as an object. If the author wants bar to be a method of foo rather than a standalone function, she should write it as such and have a default export foo with a property bar. (This isn't just an aesthetic choice — the author has no control over which import form people will use. Assuming people will use the namespace form is bad programming.)

Rollup treats the two forms as equivalent (demo 1, demo 2). If it didn't, it would have to import all of foo, including the bits that aren't used. (It does deoptimise like this if there's a dynamic lookup on foo, or if foo is e.g. exported or passed to a function.) That would suck.

The only time the two forms aren't equivalent is if bar uses this. If Rollup can analyse bar and determine that that's the case, it can a) warn and b) deoptimise. (That doesn't currently happen but it could.) But if bar looked like this...

function makeAFunction () {
  return function () {
    console.log('analyse', this);
  }
}

export const bar = makeAFunction();

...then analysis gets much harder — in some cases, it would be impossible. So Rollup is faced with a choice between a) deoptimising every time it can't fully analyse a namespace import (lots of false positives, would completely defeat tree-shaking), and b) not being 100% spec-compliant in this case, where the author wrote some bad code. The ideological choice is b, the pragmatic choice is a.

@ljharb
Copy link

ljharb commented May 18, 2017

A very very common use case is import * as m from 'path'; Object.keys(m). Does this mean that this will just fail outright in rollup?

@ljharb
Copy link

ljharb commented May 18, 2017

In other words, it seems like if you're trying to do dead code elimination, the bad pattern is import * as (which is explicitly importing the entire module), not obscure stuff with this.

@Rich-Harris
Copy link
Author

Does this mean that this will just fail outright in rollup?

It deopts, as mentioned above.

Maybe import * is also a bad pattern. (I don't think it is, since you're 'importing the entire module' either way — DCE only happens in the context of tooling, and the tooling gets to choose whether or not to treat them equivalently.) But that's orthogonal to the question of whether it's a bad pattern to write code that behaves completely differently depending on which of two almost-equivalent import forms is used.

@ljharb
Copy link

ljharb commented May 18, 2017

That's totally fair - at no point would I defend the quality of this code, nor would I write it.

However, I'm not sure why import * has any benefits unless you're reflecting on it or dynamically interacting with it, besides "I don't feel like writing out all the explicit imports" - I'd love to hear of use cases if you have some.

@ljharb
Copy link

ljharb commented May 18, 2017

Thinking about this more - using import * is an uncommon case imo, but certainly using import * for reflection is even less common - if you apply your heuristics, it seems like you'll come up with the following categories:

  1. only static imports
  2. import *, but only used statically
  3. import *, ? ? ? (some small percentage of this group uses this)

The question is then what to do in the third group - one choice is to import the whole module (correct per spec, but slower); another choice is to bail out and refuse to continue (advising them to avoid import *, or use some sort of override comment); another choice is to silently do the wrong thing for named imports using this.

I'd be very surprised if the number of people impacted by "bail out" is significant; and my suspicion is that this would be a better approach than risking silent failure in any situation. Thoughts?

@Rich-Harris
Copy link
Author

I don't think it's all that uncommon. To give an example of why it's not just laziness — say you're perfecting an animation in your UI, and you're using a library like eases-jsnext. I want to be able to try out a few different eases — sineOut, cubicOut, quintOut and so on — until I find the right fit for what I'm building.

If I use import *, then I can start with eases.sineOut, and change it by replacing sineOut with cubicOut where it's used in my code. I don't need to scroll back up to the import declaration, remove the old specifier (after checking that it's not still being used somewhere else in the file), add the new one, go back to find the point where I was previously working, and then edit that code. Not only that, but my IDE can autocomplete eases.cu... and eases.qu... etc.

Multiply that by every single time you use an export from a poly-package (Lodash, D3, etc) and you're not just talking about a few seconds saved here and there, you're talking about a fundamentally smarter way of working. And because you've opted in to a bundler that (admittedly initially through ignorance of the spec) treats those as static imports, you still get the benefits of tree-shaking, no runtime lookup, etc etc.

Even if the number of people affected by bailing out was insignificant, the bailouts themselves might not be — the one extra function that I have to import might have a gazillion transitive dependencies that I could have avoided.

Introducing an option seems reasonable, we'll (Rollup) try and get round to that at some point. But FWIW (probably not much given this ship has clearly sailed) I do think the spec ought to change here — module namespace records aren't normal objects, so I don't think there's a good reason to insist that this shouldn't be undefined inside foo.bar() if foo is a namespace, other than some hand-waving about being consistent with something that a lot of JS devs already find bewildering. But I'm not holding my breath :)

@ljharb
Copy link

ljharb commented May 19, 2017

It's something I'll definitely bring up at my next TC39 meeting - but I think that to have an object where a.b() doesn't set the this to a, when var o = { b }; o.b() would set the this to o, is indeed a very unprecedented and confusing thing - making something that devs find bewildering even less predictable :-)

One other thing to note is that the ModuleRecord is supposed to be frozen, and === with every appearance; so you'd only need to ever generate it once per source module.

@jasonkuhrt
Copy link

@ljharb Why haven't you considered the qualities of readability as a use-case for namespaced imports?

Pseudo examples

import * as React from 'react'
...
React.render
import * as JSON from 'json`

JSON.parse
import * as MyLib from 'mylib'

type settings = MyLib.Settings & { ... }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment