fix(gatsby-source-contentful): fix downloadLocal for LMDB #33715
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gatsbot
bot
added
the
status: triage needed
|
There are failing unit tests here, that I can fix, but this PR is mostly "concept" - I don't know if it's best way to implement downloading assets before creating nodes, I just did "massage" code to fit it |
pieh
added
topic: source-contentful
|
Other alternative than this approach is to keep most of it as is and just not mutate node in and instead usecreateNodeField + schema customization so that localFile seems to be working the same to users - will explore that in a bit
|
|
I'll have a look tomorrow |
|
Not sure if this will be helpful, but the change in this PR now is very similar to updated docs (not yet merged in #33720 ) that is meant for easier migration than what I initially had (which was also "correct" in sense that would work, just shuffled so much code around that it was making review much more difficult) |
axe312ger
force-pushed
the
fix/contentful-download-local
branch
from
a348995 to
9e56216
Compare
axe312ger
approved these changes
Oct 29, 2021
axe312ger
added a commit
that referenced
this pull request
Nov 9, 2021
* fix(gatsby-source-contentful): fix downloadLocal for LMDB * Revert "fix(gatsby-source-contentful): fix downloadLocal for LMDB" This reverts commit 5bacca7. * use createNodeField + schemaCustomization * tests: add downloadLocal to e2e tests * fix unit tests after removal of pre bootstrap Co-authored-by: axe312ger <opensource@axe312.dev>
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In v4 (or with LMDB rather as this does apply to v3 with LMDB/PQR experiments enabled) mutating nodes after they are created is no longer working (even before while it was "working" it often caused a lot of problems and was antipattern that could cause difficult to debug problems).
Right now when
downloadLocaloption is used in Contentful plugin, it actually does try to mutate node after it was created resulting inlocalFilejust not being there anymore (even tho files are still downloaded).This PR instead of mutating node directly, use
createNodeFieldto add data in Gatsby friendly way to a node (that works with LMDB). And then use schema customization so thelocalFileremains on root level of the "node" in GraphQL (so no breaking changes)We do have note about this problem in migration https://v4.gatsbyjs.com/docs/reference/release-notes/migrating-from-v3-to-v4/#dont-mutate-nodes-outside-of-expected-apis but
Makes it really hard for users or plugin authors to actually catch that. This will likely have follow ups to actually do add ways of automatic detection (maybe in diagnostic mode, or maybe some heuristics that checks just subset of nodes or combination of both). https://github.com/gatsbyjs/gatsby/compare/node-mutations-are-a-no-no has potential detection, but not benchmarked yet (but at least it allowed me to catch the problem here)