benjamn / reify

Enable ECMAScript 2015 modules in Node today. No caveats. Full stop.
MIT License
742 stars 29 forks source link

Live binding incomplete #180

Open jdalton opened 7 years ago

jdalton commented 7 years ago

In adding a unit test for modifying a constant value I noticed a gap.

const.js
export const value = 1
test.js
import { value } from "./const.js"

value = 2 // should throw

It should throw since value is a const, but currently reify doesn't track imported values, so doesn't wrap them in a setter call. It also defines value with either var or let but that prolly doesn't matter as much because if it were wrapped in a setter call an error would get thrown in the const module.

benjamn commented 7 years ago

From the perspective of the importing module, live bindings are supposed to be immutable regardless of how they were declared in the exporting module, so (just to be clear) the const in the first module doesn't matter here.

As you know, the Reify-compiled version of the variable has to be mutable so that the setter function can assign it any value(s). There's nothing we can do about that, so I'm afraid Node can't help us once the code has been compiled.

If you use the Babel plugin, the compiler should raise an exception in this case, because it considers value to be immutable (even though it can technically still be modified by the setter function).

Perhaps the Reify compiler should throw if it finds any assignments to imported symbols? Should that be the default behavior, and/or configurable?

jdalton commented 7 years ago

From the perspective of the importing module, live bindings are supposed to be immutable regardless of how they were declared in the exporting module, so (just to be clear) the const in the first module doesn't matter here.

Ah nice!

Perhaps the Reify compiler should throw if it finds any assignments to imported symbols? Should that be the default behavior, and/or configurable?

If it can't be caught at runtime then a compile time check seems right. Since it's spec compliance stuff I think on-by-default is good (making it configurable is dealers choice).

jdalton commented 7 years ago

For reference the output test here should error.