search

felixge / node-sandboxed-module

A sandboxed node.js module loader that lets you inject dependencies into your modules.
MIT License
342 stars 42 forks source link

support for recursive requires (creating a virtual enviornment) #27

Closed alonbardavid closed 10 years ago

alonbardavid commented 10 years ago

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).

domenic commented 10 years ago

Oh awesome!! I've wanted this for a long time; see #11. I'll have time to review the code this weekend, or @searls can do it if he has time.

I think this should be the default, not necessary to enable through options.

felixge commented 10 years ago

@Illniyar awesome, thank you so much. One question: It looks like your PR changed indention level from 2 to 4 spaces?

alonbardavid commented 10 years ago

sorry, sometimes my IDE does that. I've changed it back to 2 spaces.

alonbardavid commented 10 years ago

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?)

felixge commented 10 years ago

@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.

domenic commented 10 years ago

Code review over, lots of nitpicks, nothing substantial. Awesome to have you on the team! :D

alonbardavid commented 10 years ago

I've committed changes based on the code review comments. Let me know if I missed something.

felixge commented 10 years ago

:sparkling_heart: thx for reviewing in detail @domenic

sindresorhus commented 10 years ago

Can this be merged?

buschtoens commented 10 years ago

So... I kinda need this. Will you folks merge it or do we have to use the fork?

Poking @felixge @Illniyar @domenic :)

domenic commented 10 years ago

Merged as 350321bfa4a27959f32301a892d8c455c74005a9; 1.0 release coming soon.

buschtoens commented 10 years ago

Awesome guys. Thanks!