-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
fix(gatsby-source-contentful): Prevent TypeError if many-to-one reference already exists #17500
fix(gatsby-source-contentful): Prevent TypeError if many-to-one reference already exists #17500
Conversation
In the case of many-to-one references, the foreign reference is a string rather than an array. In this case, trying to create the reverse linkage results in a TypeError from attemtping to `push()` to a string. This checks the `push()` method can indeed be used to prevent such TypeError(s).
Hey @froddd! Thanks for this PR! We really appreciate you taking the time to open it. This PR looks good but to have more confident in moving this forward I would love to know how to test this outside of jest. In the interest of moving this along most effectively, we strongly encourage our contributors to provide an example project which we can then use to validate the changes in this PR. This will allow us to quickly and easily ensure that this does indeed address the purported feature or fix in this PR! An example project: can be run locally or credentials have been provided Thanks for this PR and thanks for using Gatsby! 💜 |
Sure thing -- example repo to showcase the issue is a straight clone of the Gatsby Contentful Starter project with one migration added: https://github.com/froddd/gatsby-contentful-starter Install as normal and link to your contentful space:
Add an
At this point, if you simply run the dev site locally ( Add an Author which has one of the posts as favourite and publish it: Now clear the cache and re-run the local dev environment:
You will get the following error:
|
Is there a way to link to this PR's Or are you happy with simply cloning my PR and using |
When linking locally this now produces another error:
I'll do more looking into this. |
Looks like that extra error could be linked to this: #16455 |
I've created a better, more stripped-down example to validate the bugfix: https://github.com/froddd/gatsby-contentful-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! I've tested it with your script and it works :ok-hand:. Contentful users will be grateful for this!
Holy buckets, @froddd — we just merged your PR to Gatsby! 💪💜 Gatsby is built by awesome people like you. Let us say “thanks” in two ways:
If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’. Thanks again! |
Published in |
Description
When creating nodes, linkages to foreign references are created. If a foreign reference is of type many-to-one, the reference is recorded as a string (normalize.js:330). When creating the reverse linkage (normalize.js:345),
createContentTypeNodes()
attempts to push to this foreign reference. As it is, in this case, a string, this results in a TypeError.The above can be tested using the following content type schema:
With one Blog post linking to one Author which itself links to the Blog Post, the node creation will fail with the following error:
Fix
I've added the above simple schema to the test data for
normalize.js
, and fixed by adding aArray.isArray()
check (this approach seems favoured earlier: normalize.js:298), for consistency, rather than checking that the reference is not a string.Snapshots have been updated.
Related Issues
Related to #3064 (possibly?)