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

discover native modules from process.moduleLoadList ... #34

Closed robrich closed 10 years ago

robrich commented 10 years ago

... instead of using a hard-coded list

domenic commented 10 years ago

Very impressive discovery; excited to have this. Lots of nits, but good stuff overall.

robrich commented 10 years ago

Changes made, Travis went green.

robrich commented 10 years ago

What I really like about using the discovered list to test them all is it's also future-proof. If they include another native module or deprecate one, the test still works. I grant it's not as discoverable, thus the excessive comments, but I do think it's a safer design.

domenic commented 10 years ago

Yeah, let's use the discovered list, but with a static checked-in fixture instead of writing a bunch of temp files.

robrich commented 10 years ago

I rebooted, and suddenly process.moduleLoadList was nearly empty. Switched to monkey patching (pillaged from https://gist.github.com/Benvie/1841241) to get NativeModule, which works like a charm. Also note that when dynamic test fails, it's much easier to know why, so I included both.

domenic commented 10 years ago

Let me be clearer: I don't want any tests in this repo that create temporary files on the disk :)

robrich commented 10 years ago

Thank you for clarifying.

ChrisEineke commented 10 years ago

Will this be re-pulled at some point? I'm running into an issue with a module loading winston-syslog, which has native code to it, on which the sandbox loaders pukes.

domenic commented 10 years ago

Yes, I've been meaning to revive this and just remove the temp-file creating tests before merging. Thanks for chiming in; that helps me prioritize.

robrich commented 10 years ago

Use proxyquire. It has no such issues.

ChrisEineke commented 10 years ago

domenic: Great. :) robrich: No. :)