Circular dependency doesn't work with anonymous function declaration

[nicolo-ribaudo]

https://github.com/nicolo-ribaudo/std-esm-bug-anonymous-fn-decl

It works with node, but not with @std/esm:

~/Documenti/anonymous-function-declaration on  master is 📦  v1.0.0 via ⬢ v8.9.2 took 23s 
•100% ➜ node --experimental-modules main.mjs 
(node:16566) ExperimentalWarning: The ESM module loader is experimental.
fn

~/Documenti/anonymous-function-declaration on  master is 📦  v1.0.0 via ⬢ v8.9.2 
•100% ➜ node -r @std/esm main.mjs
TypeError: f is not a function
    at /home/nicolo/Documenti/anonymous-function-declaration/g.mjs:2:9
    at Object.<anonymous> (file:///home/nicolo/Documenti/anonymous-function-declaration/file:/home/nicolo/Documenti/anonymous-function-declaration/file:/home/nicolo/Documenti/anonymous-function-declaration/g.mjs:1)
    at Module._compile (file:///home/nicolo/Documenti/anonymous-function-declaration/file:/home/nicolo/Documenti/anonymous-function-declaration/file:/home/nicolo/Documenti/anonymous-function-declaration/module.js:635:30)

If I give a name to the function defined in f.js or I remove the first import from main.js, it works as I'd expect.
I'm not 100% sure that this is a bug, since I don't understand well the part of the spec related to modules and rollup behaves exactly like @std/esm.

[jdalton]

Hi @nicolo-ribaudo!

Looks like I can fix this with a little import/export order juggling or name generating for the anon-function. I'll kick something around and report back.

For a temp workaround you can move the g import above f:

import g from "./g";
import f from "./f";

[nicolo-ribaudo]

Thank you!

[benjamn]

Would you expect this to work for arbitrary export default expressions? For example:

import "./module/that/imports/and/immediately/uses/default.js";
export default fnThatReturnsFn();

The fnThatReturnsFn() call could have side effects, so the JS runtime can't automatically hoist it.

In some sense,

import "./module/that/imports/and/immediately/uses/default.js";
export default function () {...}

is just another export default <expression> form, so the default export arguably should not be defined until that expression has been evaluated (after the import, without any special hoisting).

You can still import the default export before it has a value, just like f in this code:

export let f;
import "./module/that/imports/and/immediately/uses/f.js";
// Too late!
f = function () {...};

Since the expression in question isn't evaluated until after the circular import, there may not be anything the JS runtime can safely do to make this work for you.

[jdalton]

@benjamn

Would you expect this to work for arbitrary export default expressions?

It's less about what I or @nicolo-ribaudo expects to work and more about what does work in the native runtime. In this case, @nicolo-ribaudo's example does work with --experimental-modules so is something we should support.

I'll tackle it by generating a safe name, transforming the anonymous function into a safe named declaration.

[nicolo-ribaudo]

@benjamn I'd only expect declarations to be hoisted, like it has always been in JavaScript.
I wouldn't be surprised if this code doesn't work with circular dependencies, since it exports an expression and thus it isn't hoisted:

export default (function () {});

While this should be hoisted, so I'd expect it to work with dependnecy cycles:

export default function () {}

I'm not 100% sure that this is a bug, since I don't understand well the part of the spec related to modules and rollup behaves exactly like @std/esm.

rollup/rollup#1787: In rollup I fixed this issue by transforming anonymous function declarations to named function declarations.
I now zero about @std/esm implementation, but I suspect that treating them the same way might fix this bug (since if I manually give it a name, it works).

---

EDIT: @jdalton If you haven't started working on this yet, I'd like to contribute!

[benjamn]

I agree we should reconcile what @std/esm does with what Node does.

However, I understand

export default function () {}

to be exporting a function expression, which is not a declaration (function declarations can't be anonymous, right?), so I'm not following the argument for hoisting it.

[nicolo-ribaudo]

It is an anonymous function declaration:

• https://tc39.github.io/ecma262/#prod-ExportDeclaration

  ExportDeclaration:
    ...
    export default HoistableDeclaration[~Yield, ~Await, +Default]
    ...
  

• https://tc39.github.io/ecma262/#prod-HoistableDeclaration

  HoistableDeclaration[Yield, Await, Default]:
    FunctionDeclaration[?Yield, ?Await, ?Default]
    ...
  

• https://tc39.github.io/ecma262/#prod-FunctionDeclaration

  FunctionDeclaration[Yield, Await, Default]:
    ...
    [+Default] function ( FormalParameters[~Yield, ~Await] ) { FunctionBody[~Yield, ~Await] }
  

Btw, I just realized that this issue needs to be fixed also or async functions and generators.

[jdalton]

@nicolo-ribaudo

EDIT: @jdalton If you haven't started working on this yet, I'd like to contribute!

That would be so awesome 🎉 ! The place to start looking is here.

[benjamn]

Ok, I didn't realize anonymous declarations were a thing!

By the way, I think you can get this to work in @std/esm without giving the function a name, as long as the Acorn parser gets the .type right (FunctionDeclaration rather than FunctionExpression). This is important because the function could be parenthesized (as you mentioned above), in which case it really should be a FunctionExpression. Since you can't easily tell if there were parentheses by looking at the AST, you can't safely force the .type to be FunctionDeclaration. In short, you need to be able to trust the parser.

[nicolo-ribaudo]

@benjamn The name is needed because anynimous declarations are only allowed after export default, so they need an id when export default is removed.
I just checked and acorn generates the correct node types.

[jdalton]

Moved to #187.