577782 - Allow setting exported module value

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, enhancement)

Description

[James Burke]

There has been a proposal in CommonJS to allow setting the exports object to a specific value. More information is here:
http://wiki.commonjs.org/wiki/Modules/SetExports

It is already in use by some CommonJS implementations. NodeJS uses a module.exports assignment instead of a setExports function.

Provide a way to use both module.exports assignment and module.setExports() to allow setting the module value. This will allow more interoperability with some other CommonJS modules.

A patch with tests is available here, I am happy to provide it in another format if that is useful:

http://hg.mozilla.org/users/jrburke_gmail.com/jetpack-sdk/rev/26e0eb3d55d1

Comment 1

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Argh, we really dropped the ball on this one.  James: is there any chance you can put together an updated patch for it?

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Hi Myk, James,

Brian has patch for it:
https://github.com/warner/addon-sdk/commit/232a9cf395bd393cb4c3d8fa920f6f2e434049b6

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Also please note that it follows nodejs rather then commonjs:


module.exports = MyExports

Since aim is to be compatible with the libs in npm registry, also I guess setExports may be easily added on top.

Comment 5

[James Burke]

Partial support for this is in the work being done for #596932, in that the exported value can be assigned by returning from the define factory function.

My initial patch for this ticket also attempts to ensure that the exported value does not change after another module tries to access the exported value (with a unit test -- the work for #596932 also has a similar test). On the CommonJS list, there was a concern about maintaining the fidelity of a module and how it is represented to other modules, and it was suggested to throw an error if the exports changed after another module grabs a reference to it.

It is worth considering that option, but I defer to Brian on how he would like it implemented. I am happy to provide a patch for it though. I would likely do it after having #596932 resolved one way or the other.

Comment 6

[Brian Warner [:warner :bwarner]]

yeah, it's a good idea to prevent modules from playing games with their exports by changing them after "import" time. I suspect that we're going to want to freeze() the 'exports' object after the require/define code has finished: anything less makes it possible for the exporter to yank the rug out from under callers. We should look into our ES5 implementation to see if that works yet, and talk about how it might surprise calls who want to leave mutable state in their exported object. A reasonable starting point may just be to disable module.setExports() and to make a copy of the 'exports' object after load, so that e.g. 'module.exports=newobject' in a later callback won't be visible from other modules.

The only remaining weirdness I can think of is when there are cycles in the module-reference graph: in that case, while module A is being defined, if it does require("B"), and inside B it does a require("A"), then what should B get? But I think it's ok if things break in that case: I think Jetpack (both the implementation and the programs people build with them) will be simpler if we prohibit such cycles.

Comment 7

[James Burke]

I'm not sure freeze()ing the exported object would be desirable in all cases. I can see there will be cases when a module may want to augment another's export. An example that may not be completely applicable but I could imagine analogs: jQuery plugins augmenting jQuery.

I agree that given an error when a module's export is trying to be set after another module grabs its value is a reasonable thing to do. 

With the patch in bug 596932, it is also possible to "set exports" by returning from the define factory function. I have a test that tests for modules that try to ask for/use exports, but then later try to set it after another module has asked for the value:

https://github.com/jrburke/addon-sdk/blob/master/packages/api-utils/tests/test-modules.js#L84

this.pathAccessed was the approach to figuring out when a module was accessed by another module and when to throw the error:

https://github.com/jrburke/addon-sdk/blob/master/packages/api-utils/lib/securable-module.js#L199

Comment 8

[Brian Warner [:warner :bwarner]]

We should allow traditional (synchronous) modules to provide the exported object too, right? So I think we need a patch to let them do module.exports= and maybe module.setExports(x) .

Let's build that patch on top of bug 596932, since getting async 'define' is the more important feature, and the code overlaps a lot.

Comment 9

[Brian Warner [:warner :bwarner]]

https://github.com/mozilla/addon-sdk/pull/94 has the pull request, which also includes bug 391343.

Comment 10

[Brian Warner [:warner :bwarner]]

landed in https://github.com/mozilla/addon-sdk/commit/2c11b864bb8a331392b5f289b44c578c961b98c7