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

circular live bindings not working as expected? #845

Closed
trusktr opened this Issue Aug 11, 2016 · 13 comments

Comments

Projects
None yet
4 participants
@trusktr
Copy link

trusktr commented Aug 11, 2016

For example, these modules work in environments implemented with Babel, but the rollup.js code fails. Is there some way to write these circular modules so that they work in any ES2015 module environment?

For reference, the let A = A and let B = B is required in Babel environments, as pointed out by @benjamn.

I'm looking for any ideas on whether there's a solution that should theoretically work in any ES6 environment, not just with Babel.

@trusktr

This comment has been minimized.

Copy link
Author

trusktr commented Aug 11, 2016

I have the following, which I think should theoretically work in all ES2015 module systems: http://goo.gl/6zls2f

This is working in Babel-based projects (Meteor, Webpack, haven't tried Browserify). But, it fails in Rollup although I think it should theoretically work if bindings are "live". If you try the bundled code, it will fail because "C is not defined" in the first pair of conditional checks, although if the bindings were "live", then the check should not fail, but the resulting boolean should be false.

EDIT: Note that let A = A and let B = B should be replaced by let A = undefined and let B = undefined, but the problem @benjamn pointed out in Babel environments.

@trusktr

This comment has been minimized.

Copy link
Author

trusktr commented Aug 11, 2016

To understand what I mean, just run if (C) console.log(C) in your console, and you'll get an error because "C is not defined". This is what is happening in the Rollup output. Had the bindings been "live", then C would be defined by the time it is first used.

@trusktr trusktr changed the title circular live bindings work as expected? circular live bindings not working as expected? Aug 14, 2016

@Rich-Harris

This comment has been minimized.

Copy link
Contributor

Rich-Harris commented Aug 17, 2016

Without having a working implementation to test against, it's hard to know for sure if Rollup behaves the same as native modules eventually will, but I think that Rollup is behaving according to spec here: #443.

Basically, as far as I can tell (and I'd welcome clarification from anyone who knows better), the spec says that if module main imports module A, module A must evaluate before module main (uncontroversial). If A imports C, C must evaluate before A – also uncontroversial. If C imports A, which is already at this stage in the process of evaluating, then A is undefined as far as C is concerned while it's evaluating.

If you open the console on your initial example, you'll see a message like this:

screen shot 2016-08-16 at 7 40 52 pm

That's Rollup's way of telling you that we've encountered exactly that situation. A solution, as kludgy as it seems, is to import C from main before anything else gets imported – that way C is evaluated last (counter-intuitive I know!) meaning that setUpA and setUpB are able to run. Here's an example – the bundle runs fine until you comment out the initial empty import.

Babel has to transpile ES modules to CommonJS, which has different semantics (e.g. require is imperative whereas import is declarative), so it's not totally unexpected that the behaviour will differ. For example it looks like exports.default is being reassigned after the module initialises, which I think is against spec (I could definitely be wrong though).

Hope this answer generated more light than heat!

@Rich-Harris

This comment has been minimized.

Copy link
Contributor

Rich-Harris commented Aug 17, 2016

For example it looks like exports.default is being reassigned after the module initialises, which I think is against spec

Now that I think about it, Rollup is also behaving incorrectly here. A and B should be fixed, because they're default exports (default exports aren't supposed to be live). Rollup is apparently getting confused because you're doing export {default as A} rather than export default A – that's a bug. You can see what should happen here.

@Rich-Harris

This comment has been minimized.

Copy link
Contributor

Rich-Harris commented Sep 18, 2016

I'll close this as it doesn't look like anything is behaving incorrectly or that there's anything to be done here – reopen if that's not the case

@Victorystick

This comment has been minimized.

Copy link
Member

Victorystick commented Sep 23, 2016

@Rich-Harris It seems to me that you've just introduced a bug rather than fixed one. Are you saying default should never be a live binding? As far as I understand, only export default ... gets special treatment. The spec treats all export { x as y } the same. There is no special rule saying there shouldn't be a binding if the exported name is "default".

In this first example, there is no live binding. foo is neither a HoistableDeclaration nor a ClassDeclaration, meaning that it must be an AssignmentExpression which is evaluated and exported as the default.

var foo = 0;
export default foo;
foo = 1;

Live binding. The variable foo is part of an ExportClause where the ExportName of the variable is "default"; nothing else has changed. It behaves just the same as if it had been export { foo }.

var foo = 0;
export { foo as default };
foo = 1;
@trusktr

This comment has been minimized.

Copy link
Author

trusktr commented Sep 24, 2016

I've made a simpler example that shows the problem. You'll get an undefined error if you try pasting the result in your console: https://goo.gl/O7FnxM

That should theoretically work in an ES2015 Module environment because the var statements are hoisted, and the initA function (which is also hoisted) is called before the B or C classes are defined so by the time when the B or C class definitions are evaluated the value of A should not be undefined.

However, in the compiled result, a new variable A$1 is created and the A symbol is replaced in the B and C modules, so if one of the B or C modules is evaluated first, there will be an undefined error because the initA function is not setting the A$1 variable, it is setting the A variable.

@trusktr

This comment has been minimized.

Copy link
Author

trusktr commented Sep 24, 2016

@loganfsmyth (versed on ES2015 modules), can you verify this (my previous comment)?

@loganfsmyth

This comment has been minimized.

Copy link

loganfsmyth commented Sep 24, 2016

Your analysis seems right, the B class should probably stay extending A, not A$1. This example seems potentially separate from the original issue reported here though.

@trusktr

This comment has been minimized.

Copy link
Author

trusktr commented Sep 24, 2016

True, it is separate. I originally posted

I'm looking for any ideas on whether there's a solution that should theoretically work in any ES6 environment, not just with Babel.

So the new example shows the solution that I arrived at, but coincidentally still doesn't work in Rollup. I'm not sure about the original example though.

@loganfsmyth I believe you said (in an external conversation from here) that the things I'm doing in my original examples are only workarounds to how Babel compiles.

Is the following example something that would work in a theoretical ES6 environment? Or would there be a temporal deadzone error? I think it should be a temporal deadzone error: http://goo.gl/6zls2f

@loganfsmyth

This comment has been minimized.

Copy link

loganfsmyth commented Sep 24, 2016

Yes that would trigger a TDZ error.

@trusktr

This comment has been minimized.

Copy link
Author

trusktr commented Mar 9, 2017

@loganfsmyth IIRC, you had helped me solve circular dependencies by showing me an "init function" approach. If true, do you remember where that discussion was?

@trusktr

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.