fix: use content type id by default to create reference gql types #26102
Conversation
axe312ger
added
the
status: needs core review
gatsbot
bot
added
the
status: triage needed
axe312ger
force-pushed
the
contentful-next
branch
from
246dd51 to
c5bb85d
Compare
ascorbic
added
topic: source-contentful
There was a problem hiding this comment.
This was intense to review. So many loops.
Generally speaking, I'm very concerned about the runtime performance implication of this PR. Please make sure to check this with some scaling builds. 15k, up to 100k if you can. The code loops over so many pieces and I can't tell how many each piece is expected to contain on average.
Additionally please check with a large site with many locales (10+?).
Additionally, I'm concerned about models that have circular references (Contentful can produce those) crashing the plugin. Please make sure to test with a model that somehow generates those circular references. Better yet, add a test for it. In particular, all the cases that exhaustively traverse or JSON.parse an object.
I've left many comments but ultimately you are the code owner. So I'm going to accept it and leave it up to you whether you want to address the comments or not. I hope you'll address at least some concerns.
| @@ -7,16 +7,26 @@ jest.mock(`gatsby-core-utils`, () => { | |||
| } | |||
| }) | |||
|
|
|||
| const _ = require(`lodash`) | |||
| const gatsbyNode = require(`../gatsby-node`) | |||
| const fetch = require(`../fetch`) | |||
| const normalize = require(`../normalize`) | |||
|
|
|||
| const startersBlogFixture = require(`../__fixtures__/starter-blog-data`) | |||
There was a problem hiding this comment.
Nit: if this file returned a function that returned the data structure, then you wouldn't need to bother to _.deepClone the structure every time. In fact, you wouldn't need to worry about cross test pollution at all.
| .mockReturnValueOnce(_.cloneDeep(startersBlogFixture.initialSync)) | ||
| .mockReturnValueOnce(_.cloneDeep(startersBlogFixture.createBlogPost)) | ||
|
|
||
| const createdBlogEntry = | ||
| startersBlogFixture.createBlogPost.currentSyncData.entries[0] |
There was a problem hiding this comment.
Since you deep clone, the startersBlogFixture reference may hold different values than what was passed on to fetch above. Not sure if that matters here.
| @@ -134,6 +138,8 @@ ${formatPluginOptionsForCLI(pluginConfig.getOriginalPluginOptions(), errors)}`) | |||
| space, | |||
| } | |||
|
|
|||
| console.timeEnd(`Fetch Contentful data`) | |||
There was a problem hiding this comment.
I wonder if there isn't an easier way to group the start/stops of these. 🤷 But please don't remove them because I love seeing this.
There was a problem hiding this comment.
Same, I replaced all consone.timeEnd with GatsbyReporter.
| @@ -39,7 +40,8 @@ | |||
| ], | |||
| "license": "MIT", | |||
| "peerDependencies": { | |||
| "gatsby": "^2.12.1" | |||
| "gatsby": "^2.12.1", | |||
| "gatsby-plugin-sharp": "^2.6.14" | |||
There was a problem hiding this comment.
Curious: what's mandating this?
There was a problem hiding this comment.
You only need it when using traced SVGs
| if (type.name.match(/contentful.*RichTextNode/)) { | ||
| return { | ||
| nodeType: { | ||
| type: GraphQLString, | ||
| deprecationReason: `This field is deprecated, please use 'json' instead.`, | ||
| deprecationReason: `This field is deprecated, please use 'raw' instead. @todo add link to migration steps.`, |
There was a problem hiding this comment.
Is the todo supposed to be part of the string? (same below)
There was a problem hiding this comment.
I gonna replace this anyways pretty soon when updating the documentation. Thats when I was planning to update this @todo
| value && | ||
| value.sys && | ||
| value.sys.type === `Link` && | ||
| value.sys.contentful_id |
There was a problem hiding this comment.
nit:
| value && | |
| value.sys && | |
| value.sys.type === `Link` && | |
| value.sys.contentful_id | |
| value?.sys?.type === `Link` && | |
| value.sys.contentful_id |
| value.sys.contentful_id | ||
| ) { | ||
| value.sys.id = value.sys.contentful_id | ||
| delete value.sys.contentful_id |
There was a problem hiding this comment.
delete could have an adverse performance implication, especially at scale. Consider setting it to undefined or to use a different data structure if possible (like Map).
There was a problem hiding this comment.
The delete is not needed at all, plus this part might be removed then the fixId's PR is merged.
| const prepareIdsForResolving = obj => { | ||
| for (let k in obj) { | ||
| const value = obj[k] | ||
| if ( |
There was a problem hiding this comment.
Nit: Similar as commented earlier, this if could be inside the typeof check of the else branch.
| ? getField(field) | ||
| : field[defaultLocale] | ||
| function renderRichText({ raw, references }, options = {}) { | ||
| const richText = JSON.parse(raw) |
There was a problem hiding this comment.
Is it possible for this raw data model to have circular references? If so, this call will throw an error. Please make sure to check for this.
There was a problem hiding this comment.
nope it can not by definition of a Contentful API response :)
| asset: node.data.target, | ||
| getField, | ||
| }), | ||
| // Create dummy response so we can use official libraries for resolving the entries |
There was a problem hiding this comment.
added a better comment
axe312ger
force-pushed
the
contentful-next
branch
from
5a260ab to
e5487e1
Compare
axe312ger
force-pushed
the
contentful-next-use-id-by-default
branch
from
c29cb4a to
e5487e1
Compare
axe312ger
force-pushed
the
contentful-next-use-id-by-default
branch
from
e5487e1 to
bb4359a
Compare
axe312ger
added
bot: merge on green
…e performance (#27244) * fix(gatsby-source-contentful): fixed id collision in contentful entries (#23514) * fix(gatsby-source-contentful): fixed id collision in contentful entries * fix(gatsby-source-contentful): properly resolve subsequent sync calls (#15084) * fix(gatsby-source-contentful): fixed id collision in contentful entries (#23514) * fix(gatsby-source-contentful): fixed id collision in contentful entries * improve logging and clean up code (#26238) * improve comment about contentful-resolve-response usage * test(unit): remove cross test data pollution * further improve loggin by replacing console.time with GatsbyReporter * improve code style * missing file * remove logging verbosity * adjust mocked reporter for tests to pass * remove unused deep-map dependency * fix: use content type id by default to create reference gql types (#26102) * temporary console.time calls for performance measurements * perf(gatsby-source-contentful): use a map and less loops to merge old and latest sync API response * log time Contentful API processing and GraphQL node creation * upgrade dependencies and use faster node 10 versions * remove duplicate logging * allow multiple Contentful sources fixes #26875 * refactor(imports): fix dependency structure and make sharp optional fixes #23904 * align tests to match new multi source cache pattern Co-authored-by: Matthew Miller <mnmiller1@me.com> Co-authored-by: Khaled Garbaya <khaled@contentful.com>
and add support for the
useNameForIdoption