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

Ensure overridden babel config plugins array is immutable #675

Closed

Conversation

dcousineau
Copy link
Contributor

Since babelConfig in the CachingPrecompiler.prototype._factory method was a JavaScript object coming from an external source, the .push was altering the array in place. objectAssign is shallow so the plugins were being mutated, ensuring that AVA's required plugins were continually appending for each file loaded and parsed.

Using .concat ensures the original plugins array is not mutated and thus doesn't collect plugins like a hoarder.

Does not solve #674 however it was noticed during an attempt to address that issue.

Since `babelConfig` in the CachingPrecompiler.prototype._factory method was a JavaScript object coming from an external source, the .push was altering the array in place. objectAssign is shallow so the plugins were being mutated, ensuring that AVA's required plugins were continually apending for each file loaded and parsed.

Using .concat ensures the original plugins array is not mutated and thus doesn't collect plugins like a hoarder.
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @spudly to be a potential reviewer

@novemberborn
Copy link
Member

Nice catch @dcousineau!

Could you perhaps add a test as well? Thanks.

@dcousineau
Copy link
Contributor Author

@novemberborn Yeah, I was going to copy the last test in the caching-precompiler test file but am hitting the end of my day. I'll circle back in a little bit and do so.

Also IRT the build failures: it looks like npm install failed on travis?

@novemberborn
Copy link
Member

Yeah, I was going to copy the last test in the caching-precompiler test file but am hitting the end of my day. I'll circle back in a little bit and do so.

No worries. It's late here too ;-)

Also IRT the build failures: it looks like npm install failed on travis?

Wow all tests red! I guess we'll try again when you push the test.

@dcousineau
Copy link
Contributor Author

@novemberborn

@dcousineau
Copy link
Contributor Author

@sindresorhus
Copy link
Member

Yeah... I've restarted the builds.

@dcousineau
Copy link
Contributor Author

@sindresorhus what a time to be alive~

@spudly
Copy link
Contributor

spudly commented Mar 24, 2016

Nice catch, dcousineau!

Sorry you had to waste so much of your own time fixing a bug I introduced.

@dcousineau
Copy link
Contributor Author

@spudly to be fair my issue wasn't a bug it was a misunderstanding on my part, I just happened to find this digging in and trying to understand AVA's behavior :)

@vadimdemedes
Copy link
Contributor

@dcousineau Could you please add a test to ensure this won't happen in future? Would like to merge it asap ;) Thanks!

@novemberborn
Copy link
Member

Hey @dcousineau, I've taken your fix here and added a test, see #717. Hope you don't mind, we just want to try get a fix out soon :) Thanks for digging through the code and finding the bug!

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.

6 participants