support for recursive requires (creating a virtual enviornment) #27
Conversation
|
@Illniyar awesome, thank you so much. One question: It looks like your PR changed indention level from 2 to 4 spaces? |
|
sorry, sometimes my IDE does that. I've changed it back to 2 spaces. |
|
If you enable it by default you'll break backwards compatability (don't know if thats an issue in a testing oriented library, is anyone using this in live enviornments?) |
|
@Illniyar awesome, this makes the patch much easier to read. Breaking b/c is still ok I think, it's not like this library is used by a ton of people. So the focus should be to make it better / have reasonable defaults. So I just gave you push access to merge this once your're happy with it (feel free to wait if @domenic has some time to provide more feedback). If you give me your npm account I'll add you there as well. The only thing missing from my perspective is docs about the new behavior. |
| __filename: this.filename, | ||
| __dirname: path.dirname(this.filename), | ||
| module: this.module, | ||
| exports: this.exports | ||
| }; | ||
| locals.require = this._requireInterceptor() |
There was a problem hiding this comment.
This change seems unnecessary?
There was a problem hiding this comment.
this change is necessary for partial module loading support, which is required for loading modules in a cycle.
the interceptor caches exports before it finishes loading the module so that if another module requires the first module in a cycle we won't reload the module (which is how node's standard module caching works).
This means we need the exports variable assigned before we call the interceptor.
Otherwise we'll have an infinite loop if two modules require eachother in a cycle.
There was a problem hiding this comment.
OK, cool. For something that esoteric though, it would be important to include a comment so that it doesn't get "refactored" later.
|
Code review over, lots of nitpicks, nothing substantial. Awesome to have you on the team! :D |
|
I've committed changes based on the code review comments. Let me know if I missed something. |
|
💖 thx for reviewing in detail @domenic |
|
Can this be merged? |
|
Merged as 350321b; 1.0 release coming soon. |
|
Awesome guys. Thanks! |
I've added support for recursive requires - so that you can contain an entire virtual enviornment in sandbox.
That is if you sandbox a module that requires another module, the second module will also be loaded using SandboxedModule (using the same globals as the original sandboxed module)
You can enable it by passing options.recursive:true .
The new module creates a shared global state and provides it's own module cache implementation (of a sort).