Fix module contract for modules that assign exports #20

Merged
merged 1 commit into from Mar 12, 2013

Projects

None yet

2 participants

@antifuchs

This change makes sprockets-commonjs compliant with the "Module context" contract, paragraph 1.3: "[...]the object returned by "require" must contain at least the exports that the foreign module has prepared[...]".

Some code bases use the (not yet specified/clarified in CommonJS) method of assigning exports to export things. If, in these code bases, they then go on to require more code that depends on the previous module's exports, that new code won't see these exports unless we re-cache the exports here, thus breaking that paragraph above.

@antifuchs antifuchs Assign a cached module's exports before requiring another
This makes sprockets-commonjs compliant with the "Module context"
contract, paragraph 1.3: "[...]the object returned by "require" must
contain at least the exports that the foreign module has
prepared[...]".

Some code bases use the (not yet specified/clarified in CommonJS)
method of assigning exports to export things. If, in these code bases,
they then go on to require more code that depends on the previous
module's exports, that new code won't see these exports unless we
re-cache the exports here, thus breaking that paragraph above.
f94897b
@antifuchs

I was asked out-of-band to provide an example (-:

Assume two files, foo and bar; I'll use coffeescript.

File foo:

module.exports = class Foo
  openBar: ->
    Bar.sharedBar.open()

Bar = require('./bar')

File bar:

Foo = require('./foo')
module.exports = class Bar extends Foo
  constructor: ->
    super

  openBar: ->
    alert('bar called!')

module.exports.sharedBar = new Bar

As you can see, there's a dependency cycle. The spec says that file bar, as included from file foo, should have foo's class Foo defined already.

Without this change, when the constructor for Bar runs, the cache entry for Foo will still be set to {}, causing the constructor to fail. All that this change does is refresh the cache entry before executing the require'd code, thus ensuring the contract is honored (-:

@maccman maccman merged commit 2a8c073 into maccman:master Mar 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment