Closed Bug 591343 Opened 9 years ago Closed 9 years ago

add module.id for CommonJS compatibility

Categories

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

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: irakli, Assigned: warner)

References

Details

Attachments

(1 file, 1 obsolete file)

In jetpack if you put module id to the package descriptors main property it will be considered to be a program and lunched accordingly. According to commonjs main is reserved property and any module can be launched as a program and to do so you would do something like:

cfx run path/to/my/program.js

A part from that according to the commonjs there are one more additional variable in the scope of module it's called `module` and contains info regarding current module for example id. 

require(module.id) == exports // true

If module was loaded as a program later will be true

require.main == module

In all other modules require.main will be mapped to the `module` object from the firs module loaded.
(In reply to comment #0)
> In jetpack if you put module id to the package descriptors main property it
> will be considered to be a program and lunched accordingly. According to
> commonjs main is reserved property

Where does it say that?  I checked out the 1.0 and 1.1 Package specifications, and neither reserves "main" in package.json.

http://wiki.commonjs.org/wiki/Packages/1.0
http://wiki.commonjs.org/wiki/Packages/1.1


> and any module can be launched as a program
> and to do so you would do something like:
> 
> cfx run path/to/my/program.js

That seems ok to add as an enhancement to the existing behavior of |cfx run|, as long as it remains possible to execute a program via |cfx run| without any additional command line options, either because "main" is specified in package.json or because cfx assumes the default value "main.js".


> A part from that according to the commonjs there are one more additional
> variable in the scope of module it's called `module` and contains info
> regarding current module for example id. 
> 
> require(module.id) == exports // true
> 
> If module was loaded as a program later will be true
> 
> require.main == module
> 
> In all other modules require.main will be mapped to the `module` object from
> the firs module loaded.

That seems ok to me.  Atul: what do you think?
I'd like to run it by Brian to see what he thinks, b/c it could affect the object-capability flavour of Jetpack. Also, I'm not entirely sure how this will work in cross-process land. But doing something to conform to this seems possible.
(In reply to comment #1)
> (In reply to comment #0)
> > In jetpack if you put module id to the package descriptors main property it
> > will be considered to be a program and lunched accordingly. According to
> > commonjs main is reserved property
> 
> Where does it say that?  I checked out the 1.0 and 1.1 Package specifications,
> and neither reserves "main" in package.json.
> 
> http://wiki.commonjs.org/wiki/Packages/1.0
> http://wiki.commonjs.org/wiki/Packages/1.1

Update: the 1.1 spec now specifies "main" as "module that must be loaded when require(name) is called. Definition must be relative to the package description file."  I don't actually understand what that means.
Brian has expressed a willingness to tackle CommonJS compatibility issues for the SDK 0.10 release, so assigning this bug to him.
Assignee: nobody → warner-bugzilla
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
I see a couple of different issues here:

* module.id : I propose to leave out the "module" name entirely. As far as I
  can tell, the only use for module.id is to require() it later, which would
  be equivalent to giving access to our "exports" object, and if that's the
  goal, it's easier to just hand out the "exports" object directly. In
  Jetpack, we force require() statements to use static literal strings as
  arguments, so we can perform analysis on the module graph before runtime,
  so we'd disallow require(module.id) anyways. Is there any other use-case
  for module?

* cfx run path/to/foo.js : that looks nice, but I think it's low-priority: to
  turn that foo.js into a real add-on XPI, you'd need to put it in a package
  and create some more files, so I don't think it really helps the
  development process too much. It *would* be nice to be able to quickly test
  out a bunch of examples by doing "cfx run examples/1.js" instead of
  requiring a complete directory structure for each example, but it doesn't
  feel like a strong motivation right now.

* if we defer "cfx run PATH" until later, then "require.main" is moot.

So my proposal is to WONTFIX the module.id, focus this ticket on implementing
"cfx run path/to/foo.js", and defer it for later. Thoughts?
(In reply to comment #6)
> I see a couple of different issues here:
> 
> * module.id : I propose to leave out the "module" name entirely. As far as I
>   can tell, the only use for module.id is to require() it later, which would
>   be equivalent to giving access to our "exports" object, and if that's the
>   goal, it's easier to just hand out the "exports" object directly. In
>   Jetpack, we force require() statements to use static literal strings as
>   arguments, so we can perform analysis on the module graph before runtime,
>   so we'd disallow require(module.id) anyways. Is there any other use-case
>   for module?
> 
> * cfx run path/to/foo.js : that looks nice, but I think it's low-priority: to
>   turn that foo.js into a real add-on XPI, you'd need to put it in a package
>   and create some more files, so I don't think it really helps the
>   development process too much. It *would* be nice to be able to quickly test
>   out a bunch of examples by doing "cfx run examples/1.js" instead of
>   requiring a complete directory structure for each example, but it doesn't
>   feel like a strong motivation right now.
> 
> * if we defer "cfx run PATH" until later, then "require.main" is moot.
> 
> So my proposal is to WONTFIX the module.id, focus this ticket on implementing
> "cfx run path/to/foo.js", and defer it for later. Thoughts?

I do believe that main use of module object is this:

if (require.main == module) exports.runMyApp()

I believe this pattern is widely used across packages in npm to determine whether module was lunched as program or just was required by some other module. Such modules and therefor packages most likely will misbehave in jetpack world. I do agree that cfx run path/to/foo.js would be cool but can be deferred.
oh.. so you're saying that many NPM packages contain a clause like that, sort of like how many Python modules contain a "if __name__=='__main__':..." clause at the bottom?

So, we need to provide *something* as "require.main" to prevent those modules from crashing. Er, well, if we don't do anything special, then "require.main" will just show up as "undefined", which won't ever trigger that clause, right? Is that enough to safely use those modules?

I think the rule we may be converging towards (or imposing) is that top-level modules used by 'cfx' must be written specifically for jetpack: it doesn't make any sense to use a regular NPM package as a top-level thing.
Discussion with Irakli:

* the current CommonJS module spec (http://wiki.commonjs.org/wiki/Modules/1.1.1 or so) makes require.main optional and module.id mandatory

* node.js allows modules to say "module.exports=foo" to replace the exported object altogether. This isn't in the CommonJS spec (http://wiki.commonjs.org/wiki/Modules). Irakli reports that many modules in the NPM collection (http://npm.mape.me/) use this, which means that we need to support it in order to take advantage of those modules.

* many NPM modules use (require.main === module) to tell if they're being executed as a top-level program, like python's (if __name__ == "__main__") idiom. To import these modules from Jetpack code without accidentally causing them to run self-tests, we need to make sure (require.main === module) evals to false.
(In reply to comment #9)
> * node.js allows modules to say "module.exports=foo" to replace the exported
> object altogether. This isn't in the CommonJS spec
> (http://wiki.commonjs.org/wiki/Modules). Irakli reports that many modules in
> the NPM collection (http://npm.mape.me/) use this, which means that we need to
> support it in order to take advantage of those modules.

That'd be pretty handy in SDK code too, since we often want the exports object to be iterable, and making it so without this feature is clunky.
Ok, after talking with Irakli today, the plan is:

* implement "module" and the ability to set module.exports=
 * this will cause problems for import cycles, since you can't find out what
   the export will be until the module finishes executing. This is why
   CommonJS wasn't happy about the feature (which comes from node.js). We
   probably want to discourage import cycles anyways.
* do not define "require.main" yet: put that off for a future enhancement to
  let you use 'cfx run foo.js'. The common practice in CommonJS-land is to do
  things like 'node test/all.js', which imports-and-runs a bunch of other
  unit-test modules. Perhaps eventually we could make 'cfx test' an alias for
  'cfx run test/all.js' or something.
* define "module.id" as $PACKAGE/$MODULE, whatever is easiest. If it's too
  hard, don't both: apparently nobody else uses it, despite the spec saying
  its mandatory.
* do not define "module.filename". Apparently some node.js modules depend
  upon it ("to figure out path", not sure what that means), but not enough to
  worry about for now

None of these are beta-blockers: they all relate to the ability to use
modules from a package repository like NPM.
Blocks: 614707
Attached patch patch from pull-request 70 (obsolete) — Splinter Review
Attachment #494576 - Flags: review?
Attachment #494576 - Flags: review? → review?(rFobic)
Hey Brian it looks good by me but, I also was recently CC'ed on this one: 
https://bugzilla.mozilla.org/show_bug.cgi?id=577782

Not sure if we want to add `setExports` as well so it's better to check that with Myk.
Also I don't think I'm in title of granting reviews, so I guess someone else will have to grant it, but in case I am r=me
Ok, it looks like the big item (module.exports= and module.setExports(x) ) have been moved out to bug 577782. So the remaining item here is adding 'module.id'. Since bug 596932 is rewriting a lot of the module-handling code, I'm going to mark this one as dependent on that one. I'm also updating the title.
Depends on: 596932
Summary: jetpack programs are different from commonjs programs → add module.id for CommonJS compatibility
Attached file pull request 94
this includes fixes for both bug 591343 (module.id) and bug 577782 (module.exports=obj and module.setExports(obj))
Attachment #494576 - Attachment is obsolete: true
Attachment #504961 - Flags: review?
Attachment #504961 - Flags: feedback?(jrburke)
Attachment #494576 - Flags: review?(rFobic)
Attachment #504961 - Flags: review? → review?(rFobic)
That was supposed to be a redirect to https://github.com/mozilla/addon-sdk/pull/94
Comment on attachment 504961 [details]
pull request 94

> /* if they used module.exports= or module.setExports(), get
> the new value now. If they used define(), we must be
> careful to leave self.modules[path] alone, as it will have
> been modified in the asyncMain() callback-handling code,
> fired during sandbox.evaluate(). */

Shouldn't the code then check for self.defineUsed[basePath]?  I don't know the new async code very well, so it's unclear, and maybe I'm missing something, but it's strange that this comment suggests that self.modules[path] must be left alone if define() was used but then doesn't check whether or not define() was used before messing with self.modules[path].

Otherwise, this looks fine.  r=myk assuming that I just misunderstand, but please clarify!
Attachment #504961 - Flags: review?(rFobic)
Attachment #504961 - Flags: review+
Attachment #504961 - Flags: feedback?(rFobic)
yeah, that's a good idea. working on an update now..
ok, updated added. I'll land this now. Thanks!
landed in https://github.com/mozilla/addon-sdk/commit/2c11b864bb8a331392b5f289b44c578c961b98c7
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #504961 - Flags: feedback?(rFobic)
Attachment #504961 - Flags: feedback+
Comment on attachment 504961 [details]
pull request 94

clearing very very old f? flags
Attachment #504961 - Flags: feedback?(formerly-j-r-burke)
You need to log in before you can comment on or make changes to this bug.