Wrapper script not robust to require handler changes #16

Closed
aseemk opened this Issue Jul 6, 2011 · 7 comments

Projects

None yet

2 participants

@aseemk
Contributor
aseemk commented Jul 6, 2011

Hey there,

Great work on the library, first of all! I love the intent and design behind it.

We just tried using it, though, and unfortunately we ran into one snag: we rely on another library which replaces the ".js" require() extension handler, so node-dev's require watcher doesn't get invoked on any requires from that point on.

Here is the relevant code from that library:

https://github.com/Sage/streamlinejs/blob/master/lib/compiler/register.js

You can see it replaces require.extensions[".js"], so node-dev, which set it before, no longer gets notified of .js requires. (It also does this to Coffee.)

Unfortunately, it's not straightforward to fix this from the library's side. Because of the way require.extensions works, this library can't be expected to call the previous handler after it does its thing, because the other handler could then double-call module._compile().

Do you know of another way node-dev could watch required files? Is it out of the question to actually wrap global.require instead of registering extension handlers? (That would also allow it to handle other extensions beyond .js and .coffee too!)

Thanks for your consideration, and great work again!

@fgnass
Owner
fgnass commented Jul 7, 2011

I see ... wrapping global.require sounds like a good alternative to me. I'm afraid I won't find the time to try this out in the near future, so pull requests for this issue are highly welcomed ;)

@aseemk
Contributor
aseemk commented Jul 7, 2011

Cool, I'll submit one soon. Just began working on this change, and I do have one question: what is the purpose of this block? https://github.com/fgnass/node-dev/blob/master/wrapper.js#L49-L53

I get that it's telling Node, in some way, "hey, the main file is this guy, not this wrapper script," but what implications does this have?

Since I'm now wrapping require() directly, I don't have access to (or at least, I don't know of a way I can get access to) a module object to maintain this feature. Here's what the code looks like now:

/** Hook into `require()` */
var origRequire = global.require;
global.require = function(path) {
  watch(origRequire.resolve(path));
  origRequire(path);
};

(And yeah, I've changed watch() to take a file path as an argument instead of a module object.)

Thoughts appreciated! Thanks. =)

@aseemk
Contributor
aseemk commented Jul 7, 2011

Unfortunately, this might not be possible. It seems that even though I'm overriding global.require, subsequent modules never pick up the wrapped require().

I would expect this if the NODE_MODULE_CONTEXTS flag was set:

https://github.com/joyent/node/blob/master/lib/module.js#L40-L42
https://github.com/joyent/node/blob/master/lib/module.js#L365-L395

But since that's not the default, and I haven't set it, I'm not sure why this is happening. I haven't been able to understand how compiling works fully, but it might be that a redefined require is used on every load:

https://github.com/joyent/node/blob/master/lib/module.js#L345-L347
https://github.com/joyent/node/blob/master/lib/module.js#L405-L406

Alas, this may be a dead end. You can see my changes for now:

aseemk@master...feature/require

The real solution long-term would be for Node to improve the require extension system so that handlers like node-dev's don't get overridden. I've started a thread on the Node.js mailing list for this:

http://groups.google.com/group/nodejs/browse_thread/thread/a372ff46dabc5239

Thanks anyway for your openness and consideration! I'd love to use node-dev sometime; let's hope this pans out.

@fgnass
Owner
fgnass commented Jul 8, 2011

What about this idea: After calling the original extensionHandler we check if our hook is still in place. If not, we register ourself again, wrapping the new handler which has been set the required module. Our own code has to be reentrant, so that we don't end up in an infenite loop in case the other hanler does similar things.

What do you think?

Am 07.07.2011 um 20:04 schrieb aseemk reply@reply.github.com:

Unfortunately, this might not be possible. It seems that even though I'm overriding global.require, subsequent modules never pick up the wrapped require().

I would expect this if the NODE_MODULE_CONTEXTS flag was set:

https://github.com/joyent/node/blob/master/lib/module.js#L40-L42
https://github.com/joyent/node/blob/master/lib/module.js#L365-L395

But since that's not the default, and I haven't set it, I'm not sure why this is happening. I haven't been able to understand how compiling works fully, but it might be that a redefined require is used on every load:

https://github.com/joyent/node/blob/master/lib/module.js#L345-L347
https://github.com/joyent/node/blob/master/lib/module.js#L405-L406

Alas, this may be a dead end. You can see my changes for now:

aseemk@master...feature/require

The real solution long-term would be for Node to improve the require extension system so that handlers like node-dev's don't get overridden. I've started a thread on the Node.js mailing list for this:

http://groups.google.com/group/nodejs/browse_thread/thread/a372ff46dabc5239

Thanks anyway for your openness and consideration! I'd love to use node-dev sometime; let's hope this pans out.

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

@aseemk
Contributor
aseemk commented Jul 8, 2011

Unfortunately, our handler doesn't get overridden when we call the original extensionHandler; it gets overridden the moment you require another library that wants to register an extension handler.

Here's a timeline:

  1. Node registers a (default) .js handler (code).
  2. Node-dev overwrites Node's .js handler with its own, but it calls the original (default) handler whenever it's invoked.
  3. Streamline overwrites node-dev's .js handler with its own (code), but it doesn't call the previous (node-dev's) handler when it's invoked.

Node-dev calls the previous (default) handler when its handler is invoked because node-dev doesn't care about doing anything to the code; it just wants to "know" when a file is required.

Streamline can't call the previous handler when its handler is invoked because Streamline does want to do something to the code, so it can't risk previous handlers doing further things to the code.

And node-dev doesn't get notified when Streamline overwrites its handler, because it's just a property change, not a function call.

But that got me thinking -- maybe node-dev can get notified, by replacing require.extensions['.js'] with an ES5 getter/setter property that'll notify node-dev whenever require.extensions['.js'] is changed. I'll experiment with this when I get a chance. =)

fingers crossed

@aseemk
Contributor
aseemk commented Jul 11, 2011

Oh man! I got it!

While studying how you support CoffeeScript, I realized that when you call the original require.extensions handler, it actually executes the file -- so you can immediately check right afterwards if node-dev's extension handler has been overwritten! (And if so, reset it.)

I've tested this thoroughly on my end and it works like a charm. I'll submit a pull request. =)

@fgnass
Owner
fgnass commented Jul 11, 2011

Closed by b3c1231

@fgnass fgnass closed this Jul 11, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment