Closed Bug 577782 Opened 10 years ago Closed 9 years ago

Allow setting exported module value

Categories

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

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: formerly-j-r-burke, Unassigned)

References

Details

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
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".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: Trunk → unspecified
Argh, we really dropped the ball on this one.  James: is there any chance you can put together an updated patch for it?
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: -- → 1.0b2
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.
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.
Depends on: 596932
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.
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
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.
landed in https://github.com/mozilla/addon-sdk/commit/2c11b864bb8a331392b5f289b44c578c961b98c7
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.