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

Uses Object.isExtensible() to identify if require is extensible #15

Closed
wants to merge 1 commit into from
Closed

Conversation

jishi
Copy link

@jishi jishi commented Feb 9, 2016

I stumbled upon an issue where dependant libraries wouldn't allow them to be rewired, I guess they exposed objects that had been freezed or similar.

I switched to a Object.isExtensible() identification instead, which seemed to have resolved it.

I also added the with functionality since we used that in our project.

@TheSavior
Copy link
Owner

@jishi
Copy link
Author

jishi commented Feb 9, 2016

Well, both cases return true however:

Object.isExtensible({}) => true
Object.isExtensible(function () {}) => true

However, that check would also catch undefined, which seems to throw error in node 0.12, but false in 4.0:

Object.isExtensible(undefined) => false (node 4.0.0)
Object.isExtensible(undefined) => (TypeError: Object.isExtensible called on non-object, 0.12)

So maybe an undefined check is in place to support 0.12 and earlier if that is a target version.

@TheSavior
Copy link
Owner

I believe it doesn't throw only for undefined in 0.12

> Object.isExtensible(4)
TypeError: Object.isExtensible called on non-object
> Object.isExtensible("")
TypeError: Object.isExtensible called on non-object
> Object.isExtensible(true)
TypeError: Object.isExtensible called on non-object

I believe that is why the function / object check is there.

@jishi
Copy link
Author

jishi commented Feb 9, 2016

Wow, that version... well, another option would be to use instanceof Object:

({}) instanceof Object => true
[] instanceof Object => true
(function () {}) instanceof Object => true
1 instanceof Object => false
"" instanceof Object => false
undefined instanceof Object => false
null instanceof Object => false

I guess either way is fine, but I like that all of array, object and function all are instance of object, which is what we want to test anyway.

@TheSavior
Copy link
Owner

I'd prefer to stay consistent with other similar rewire packages.

See this thread for more context: jhnns/rewire#83

Edit: Since that PR was merged, we could probably just use that file.

@TheSavior
Copy link
Owner

Can you try and see if this PR works for you? #16

@jishi
Copy link
Author

jishi commented Feb 11, 2016

Hi, sorry about the delay. That PR seem to work for me and my use case! Looks a lot cleaner too.

@jishi jishi closed this Feb 11, 2016
@TheSavior
Copy link
Owner

Sorry, I forgot about this. The fix has been pushed in 1.0.10.

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.

None yet

2 participants