Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add parent to module constructor #24

Closed
wants to merge 4 commits into from
Closed

Conversation

oroce
Copy link
Contributor

@oroce oroce commented Sep 5, 2013

@domenic
Copy link
Collaborator

domenic commented Sep 5, 2013

Test please?

@oroce
Copy link
Contributor Author

oroce commented Sep 5, 2013

Damn you silly git add -u.
Im gonna commit as soon as i get back to my computer.

@oroce
Copy link
Contributor Author

oroce commented Sep 5, 2013

test attached.


var exports = SandboxedModule.load('../fixture/parent');
console.log("exports", exports)
assert.ok(exports.module.parent);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should exports.module.parent be? Are there any properties of it you can test?

@oroce
Copy link
Contributor Author

oroce commented Sep 5, 2013

currently module.parent is node-sandboxed-module. But it might should be the parent module of node-sandboxed-module?

@domenic
Copy link
Collaborator

domenic commented Sep 5, 2013

Shouldn't it be the module that requires the module under test?

@oroce
Copy link
Contributor Author

oroce commented Sep 6, 2013

it is now.

@domenic
Copy link
Collaborator

domenic commented Sep 6, 2013

I don't think I'm quite communicating clearly.

Let's say you have module A which requires module B.

Then you sandboxed-module require module A.

Inside module B, module.parent should be module A, just as if you had non-sandboxed-module-required module A.

From what I can tell of your patch, it makes module.parent for every module in the tree be simply sandboxed-module/lib/sandboxed_module's module.parent, instead of giving each their own specific module.parent depending on their place in the tree.

@oroce
Copy link
Contributor Author

oroce commented Sep 7, 2013

i see your point, but what is the use case.

sandbox-module won't load the tree, just loads the module which you required via SandboxedModule.require(<filename>).

It would be a use case, if sandboxed-module would require fake modules, but it doesn't.
Gist: https://gist.github.com/oroce/6474892

As you can see fake module's are required by test.js.

@domenic
Copy link
Collaborator

domenic commented Sep 8, 2013

Ah, you are of course right! Thank you for having the patience to walk me through it. Merging momentarily, and will push a new release.

@domenic
Copy link
Collaborator

domenic commented Sep 8, 2013

Merged as c3df0b5 with a slightly updated test; thanks!

@domenic domenic closed this Sep 8, 2013
@oroce
Copy link
Contributor Author

oroce commented Sep 8, 2013

cool, thx.

@oroce oroce deleted the module.parent branch September 8, 2013 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants