This repository has been archived by the owner. It is now read-only.

Implement a subset of the AMD #1173

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
9 participants

fjakobs commented Jun 12, 2011

Implement a subset of the AMD spec http://wiki.commonjs.org/wiki/Modules/AsynchronousDefinition

the 'id' argument is ignored and only one define call
per module is allowed.

Implement a subset of the AMD spec <http://wiki.commonjs.org/wiki/Mod…
…ules/AsynchronousDefinition>

the 'id' argument is ignored and only one define call
per module is allowed.

ry commented Jun 12, 2011

@isaacs, can you review? I've been slowly convinced that we should do this.

@ry ry closed this Jun 12, 2011

ry commented Jun 12, 2011

See also #1170

@ry ry reopened this Jun 12, 2011

Gozala commented Jun 12, 2011

@ry @fjakobs I think my patch was implementing just a minimal subset of this. Since this is landed I'll happily close #1170 in favor of this one.

Thanks!

tj commented Jun 12, 2011

why?

isaacs commented Jun 12, 2011

@Gozala @fjacobs @ry /cc @kriskowal

I've got an implementation in my "AMD" branch, which provides compatibility with the other AMD implementations out there, without getting into the underspecified semantics of the id and dependencies arguments. Also, it works with NODE_MODULE_CONTEXTS mode, and passes all tests. I still need to add some more tests and docs to explain and validate the behavior, but in my cursory poking, this seems to work pretty good and deliver what we want.

Here are the relevant commits so far:

isaacs/node@861ef0a
isaacs/node@3841d61

The goal of this should be to be compatible with modules written for AMD systems, while adding the minimum necessary API surface to node. Towards that end, I think that doing anything special with the id or deps arguments is misguided. Only the last argument matters for node's purposes. Since require() is synchronous anyway, there is no advantage to pre-loading the dependencies. (Clearly, for systems that load modules asynchronously, this is a much more relevant point.)

tj commented Jun 12, 2011

is this going to be opt-in or will all node modules be shifting to this api?

isaacs commented Jun 12, 2011

Strictly opt-in. The goal is simply for modules written for AMD to not have to sniff for the presence of a define function and add boilerplate. I am strongly opposed to anything that requires a wholesale rewrite of every node module.

rauchg commented Jun 12, 2011

+1 for opt-in

isaacs commented Jun 12, 2011

Really, it should only be a problem if you previously depended on a global thing called define, and even then, you can still do global.define to access that, since it's implemented as a free var instead.

tj commented Jun 12, 2011

fewf, then im +1 in general

chjj commented on 6091b89 Jun 12, 2011

src/node.cc needs to be altered to compensate for the wrapper change:

node/src/node.cc

Line 1280 in 6091b89

// HACK HACK HACK

fjakobs commented Jun 12, 2011

This is meant at purely opt-in to make client server code sharing easier

On Sunday, June 12, 2011, visionmedia
reply@reply.github.com
wrote:

fewf, then im +1 in general

Reply to this email directly or view it on GitHub:
joyent#1173 (comment)

bmavity commented Jun 12, 2011

Forcing an implementation of only define(function(require, module, exports) {}) causes more problems than it's worth. This is supposed to assist people wanting to share their client/server code and it only ends up hindering them.

Gozala commented Jun 12, 2011

@bmavity That is not true, all the current implementations support that, it's actually sweet spot that works across many diff implementations. Also I don't think more than that is necessary, as it will raise another wave of debates.

tj commented Jun 12, 2011

if you know before-hand which code will be used in the page I think there is little reason to use something like this, but I guess for lazy loaded stuff it's fine

bmavity commented Jun 12, 2011

I could be missing something. How would you do dojo-style loading with this now? If you can, that's fine. But if you can't, you're now unnecessarily limiting browser code to using sync require or some ugly ass futures or whatever. There's a way to code this where any of the styles are supported.

Since node.js theoretically doesn't care about browser code, they shouldn't be choosing what style "wins"

Gozala commented Jun 12, 2011

That is a subset of AMD spec, that is well supported by requirejs, which is also known to be working with dojo: http://dojotoolkit.org/reference-guide/releasenotes/1.6.html

Also I think it's better to discuss implementation details in the AMD mailing list.

Node does care about developers though who want to write modules that can be loaded without hazards both in browser and node. Also, I think it's better to take this discussion to the mailing list: https://groups.google.com/forum/#!topic/nodejs-dev/DJz3OYGRnmg

bmavity commented Jun 12, 2011

Yeah, I figured this wasn't a good place for this. Sorry about that. :)

@isaacs lgtm +1

isaacs added a commit to isaacs/node-v0.x-archive that referenced this pull request Jun 13, 2011

@isaacs isaacs closed this in 9967c36 Jun 13, 2011

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.