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

Enable mocking of const #117

Merged
merged 7 commits into from Nov 11, 2017
Merged

Conversation

JvJefke
Copy link
Contributor

@JvJefke JvJefke commented Oct 20, 2017

This PR enables the mocking of const variables.
It uses Babel to parse the code and transform const to let variables.

It can solve the following issues: #95, #79

@JvJefke
Copy link
Contributor Author

JvJefke commented Oct 20, 2017

The tests are failing because of the fact that older node versions don't support ES6 syntax. Is there a way to only run these added tests in node 6.x versions?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling d1ad67a on JvJefke:feature/const-let-conversion into fff5037 on jhnns:master.

@JvJefke
Copy link
Contributor Author

JvJefke commented Oct 30, 2017

@jhnns Could you give some input here? thx!

Copy link
Owner

@jhnns jhnns left a comment

Choose a reason for hiding this comment

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

Wow, awesome PR! You even added tests 👍

I'm definitely willing to merge this. However, I think we can get rid of the option. I think it makes sense to apply the transform in any case because mutating const is actually a SyntaxError. In this case, babel would complain anyway. What do you think?

"coverage": "istanbul cover ./node_modules/mocha/bin/_mocha"
},
"dependencies": {
"babel-core": "~6.26.0",
Copy link
Owner

Choose a reason for hiding this comment

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

Any reason why you're not using the ^ operator? Personally, I prefer that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally prefer to use fixed or ~. A package should not contain breaking changes on minor updates but it often does. I find it more secure to make it fixed or set it to patch updates. Let me know if I still need to change is :).

@JvJefke
Copy link
Contributor Author

JvJefke commented Nov 7, 2017

You're right about the option. Hadn't thought about that. I'll remove the option when I have time.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling c0e1b87 on JvJefke:feature/const-let-conversion into fff5037 on jhnns:master.

@JvJefke
Copy link
Contributor Author

JvJefke commented Nov 8, 2017

@jhnns I removed the options. It will now always use babel to convert const to let. I had to keep the old functionality though for .coffee scripts. Babel isn't able to convert it properly. I'll try to add the functionality for coffee scripts in the future.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 501e4c2 on JvJefke:feature/const-let-conversion into fff5037 on jhnns:master.

it("Should be possible to mock a const variable using __with__ syntax", function() {
var ES2015Module = rewire("./ES2015Module", {
convertConst: true
});
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like you've forgotten this, but I'll clean it up, thanks :)

@jhnns jhnns mentioned this pull request Nov 11, 2017
@jhnns jhnns merged commit 501e4c2 into jhnns:master Nov 11, 2017
@jhnns
Copy link
Owner

jhnns commented Nov 11, 2017

Awesome 👍
Published as 3.0.0

@vekexasia
Copy link

Hello, Any reason why this shouldn't work with a code written in typescript?

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

4 participants