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

use alternative Set concatenation syntax to avoid babel spread bug #2680

Merged

Conversation

jasonphillips
Copy link
Contributor

This is a one-line PR to fix an unusual babel issue created by the Set() syntax added in #2452

Background: that prior PR worked and tested without issue, and was fine in my local application when running against a source copy of gatsby. But when installing the latest gatsby release normally, the recursion prevention was behaving very differently.

After much headache, I finally tracked it down to mangled syntax coming out of babel when using the loose:true option and creating a Set() using spread syntax. The tests were running fine, as was anything against the src/, but the dist/ created by babel was distorting the line in question.

This line (https://github.com/jasonphillips/gatsby/blob/3e96d498cc42b0268c7aa8e113236e0346eab5b2/packages/gatsby/src/schema/infer-graphql-input-fields-from-fields.js#L47):

const nextTypeMap = new Set([...typeMap, type])

Was being converted by babel to this:

const nextTypeMap = new Set([].concat(typeMap, [type]));

-- and the latter does not equate to the former. Since the erroneous code is only in the babel dist/ output, the jest testing wasn't running into it. This mangling only occurs when loose mode is set to true. I documented a minimal reproduction of this issue here in a gist: https://gist.github.com/jasonphillips/57c1f8f9dbcd8b489dafcafde4fcdba6

The alternative syntax in this PR survives babel's transformation unchanged, and Array.from() should be perfectly safe in all versions of node supported.

@gatsbybot
Copy link
Collaborator

Deploy preview ready!

Built with commit 93dc31f

https://deploy-preview-2680--gatsbygram.netlify.com

@KyleAMathews
Copy link
Contributor

Woah… sorry about having to track down such an obscure bug! Thanks for finding it though. Sounds like a bug that should be reported upstream to Babel?

@KyleAMathews KyleAMathews merged commit 4c3e8b8 into gatsbyjs:master Oct 30, 2017
@jasonphillips
Copy link
Contributor Author

So, for further reference, I did put in an issue on babel for advice / response to this issue, which generated some useful discussion:

babel/babel#6649 (comment)

My takeaway is that under loose mode, this kind of syntax problem can creep into the code and test perfectly, but effectively create a hidden bug in the distribution code where the resulting syntax is functionally different from the source. The only air-tight suggestion so far for avoiding it (in a project of this size, where it may not be feasible to ensure spread operator is never applied to a Set / Map / other built-in type somewhere) is to simply reconfigure the project's tests to run against the transpiled distribution code that will be shipped.

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.

4 participants