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

Should not try to attach to modules that export primitives #83

Merged
merged 1 commit into from
Nov 30, 2015

Conversation

elicwhite
Copy link
Contributor

When people use rewire, they wouldn't typically try to explicitly rewire modules that export primitives.

However, since reweireify, rewire-global, and babel-plugin-rewire all get injected to every module, this needs to not blow up on objects that can't be attached to.

rewireify and rewire-global use Object.isExtensible instead of the type check. The behavior of Object.isExtensible change from es5 to es6. In ES5, it would throw if given something that was not an object. in ES6, it returns false. rewireify and rewire-global all run in the browser and all modern browsers have the es6 version of the functionality. Node 0.12 has the es5 functionality, so we need to use the type check instead.

I added a test to make sure that it doesn't blow up on modules that return sealed objects. Object.defineProperty will run and not throw, but won't actually modify the object.

@elicwhite
Copy link
Contributor Author

This PR makes all of my tests pass for rewire-global while reusing getImportGlobalsSrc and getDefinePropertySrc.

@elicwhite
Copy link
Contributor Author

@jhnns I rebased this from master. What are your thoughts on this? I'd like to be able to make these modules share more and have a consistent API.

@elicwhite
Copy link
Contributor Author

@jhnns Bump.

@jhnns
Copy link
Owner

jhnns commented Nov 30, 2015

In ES5, it would throw if given something that was not an object

Does this mean that null will throw on node 0.12?

@jhnns
Copy link
Owner

jhnns commented Nov 30, 2015

Btw: Thx for your PR 👍. I'll merge it asap.

@elicwhite
Copy link
Contributor Author

$ nvm use 0.12
Now using node v0.12.7
$ node
> Object.isExtensible(null)
TypeError: Object.isExtensible called on non-object
    at Function.isExtensible (native)
    at repl:1:8
    at REPLServer.defaultEval (repl.js:132:27)
    at bound (domain.js:254:14)
    at REPLServer.runBound [as eval] (domain.js:267:12)
    at REPLServer.<anonymous> (repl.js:279:12)
    at REPLServer.emit (events.js:107:17)
    at REPLServer.Interface._onLine (readline.js:214:10)
    at REPLServer.Interface._line (readline.js:553:8)
    at REPLServer.Interface._ttyWrite (readline.js:830:14)
> 
(^C again to quit)
> 
$ nvm use node
Now using node v5.0.0
$ node
> Object.isExtensible(null)
false

@elicwhite
Copy link
Contributor Author

Good catch. Added a test case for null.

jhnns added a commit that referenced this pull request Nov 30, 2015
Should not try to attach to modules that export primitives
@jhnns jhnns merged commit e124971 into jhnns:master Nov 30, 2015
@jhnns
Copy link
Owner

jhnns commented Nov 30, 2015

Shipped with 2.5.1

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