gatsby icon indicating copy to clipboard operation
gatsby copied to clipboard

CTF-next: Generate GraphQL schema based on Contentful Content Types

Open axe312ger opened this issue 4 years ago • 30 comments

Discussion for the upcoming major release: https://github.com/gatsbyjs/gatsby/discussions/38585

Note for reviewers:

  • Follow up to #12816 and will be a breaking, so major release.
  • This branch will not be directly merged into master as some follow up PRs will come to add more features. See https://github.com/gatsbyjs/gatsby/issues/31385
  • The schema.sql for the Contentful e2e test page has about 80 additions and over 300 deletions

This adds schema generation to the gatsby-source-contentful plugin. This will remove the most annoying restriction of the plugin: You now can have fields without a single instance in your content model and your GraphQL will no more fail because of this. See: https://www.gatsbyjs.com/plugins/gatsby-source-contentful/#restrictions-and-limitations

Goals

  • [x] types will be generated based on the content model in Contentful. Your project will no more break when a field has no values at all.
  • [x] Contentful Rich Text references will always be available and allow __typename and contentful on root level
  • [x] JSON fields are now real GraphQL JSON fields. This avoids the There are conflicting field types in your data. warning and allows the JSON data structure to be changed in Contentful without breaking the Gatsby GraphQL Query
  • Create child nodes only when really necessary
    • [x] RichText fields now use a simple object type
    • [x] Location fields now use a simple object type
    • [x] (Long) Text fields still use a node yet, as gatsby-transformer-remark relies on it. Lets discuss later on.
  • [x] Keep data from Contentfuls Common resource attributes within the sys property (solved in #31007)
  • [x] Current state does break warm builds. I need to have a another look at reference resolver (solved in #31007)
  • [x] Overall align the schema closer to the GraphQL Content API (first steps in #31007)
    • Assets should have meta data like width on top level (#31115)
    • Rich Text fields split up references in entries and assets (#31122)
  • [x] Rename internal types with a scheme that avoids name collisions with content type names (#31286)
  • [x] update e2e-test/contentful (first steps in #31007)
  • ~update gatsby-remark-images-contentful to match new asset structure~ no changes needed
  • [x] update gatsby-transformer-sqip to match new asset structure (as it is part of e2e-test/contentful)
  • [x] the tags feature is now enabled by default
  • [x] https://github.com/gatsbyjs/gatsby/pull/31395
  • [x] The id generation is now done with two functions which slightly differ. This creates issues with references. WIP fix in https://github.com/gatsbyjs/gatsby/tree/feat/contentful-schema-generation-normalize-id-generation

Performance improvements

  • [x] These changes will reduce the node count which will lead to improvements in bigger projects.
  • [x] Rich Text fields no more need to JSON.stringify() and rich text renderer no more needs to JSON.parse()

Upcoming goals in the next major / Follow up PRs:

See #31385

Migration steps / Breaking changes

See https://github.com/gatsbyjs/gatsby/issues/31385#issuecomment-839640682 for the full list of steps to migrate.

axe312ger avatar Apr 13 '21 15:04 axe312ger

What do you think about adding a metadata: { ...assetItem.metadata }, to const assetNode?

Or should we make Tags a separate Node?

itwasmattgregg avatar Apr 14 '21 02:04 itwasmattgregg

@itwasmattgregg I'd like to duplicate the pattern the Contentful GraphQL API implements this new feature.

To keep reviews doable, please keep in mind that this will be in a follow up PR

axe312ger avatar Apr 14 '21 08:04 axe312ger

Decided to move #31395 out of this PR as we might need more time to get the information published through Contentful's CDA API.

This means I will try to finish #31286, get all CI green ✅ and we might do a first canary 🤔

axe312ger avatar Jun 10 '21 15:06 axe312ger

That's awesome! Have been using gatsby and contentful for years and this has been a major blocking that made us switch back to gatsby-source-graphql.

Thanks guys. There is any way we can help to test or even contribute with this feature?

lucashfreitas avatar Jul 08 '21 04:07 lucashfreitas

@lucashfreitas thanks, that schema issue also goes on my nerves for years already. I will be so happy when I add this version to my projects.

We should be soonish ready for first reviews and tests. I can mention you here as soon we are ready :)

axe312ger avatar Jul 08 '21 09:07 axe312ger

Awesome @axe312ger . Please let me know when we ready for reviews - I have some large websites using this plugin and I am pretty keen to help and test it :)

lucashfreitas avatar Jul 08 '21 13:07 lucashfreitas

Hey @axe312ger, thank you very much for this again :) Do you have any idea on when it will be ready to test? I am working on in a big website atm 1500 + pages that is using gatsby-source-graphql and I will migrate it over to gatsby-source-contentful once this feature is available to test. :)

lucashfreitas avatar Jul 15 '21 23:07 lucashfreitas

@lucashfreitas we still clean up the codebase of the plugin to allow this change to be seamless as possible. So a few more weeks for sure. I badly hope for this summer, I want to use it in my personal projects as well 😅

axe312ger avatar Jul 16 '21 09:07 axe312ger

Awesome, thanks @axe312ger . As soon as it's live we will update all of our websites to use it in favor of gatsby-source-graphl. :)

lucashfreitas avatar Jul 18 '21 23:07 lucashfreitas

After we fixed some other critical issues with the plugin (especially asset downloads), I am now back to this!

The most relevant PRs are rebased now, left for this one:

  • [x] gatsby-node.js unit test fails - looks like I broke some node creation / resolution
  • [x] downloadLocal is broken

To simplify review, I created https://github.com/gatsbyjs/gatsby/pull/34032 as a sidecar PR. It does not contain any touches to the tests, that way reviewing the actual changes should be way easier :)

axe312ger avatar Nov 19 '21 09:11 axe312ger

Even when CI disagrees. I got unit and e2e green on my machine :)

axe312ger avatar Feb 03 '22 08:02 axe312ger

Hey @axe312ger, I've been keeping an eye on this mr for quite some time. The development seems to have come to a standstill, is this still being pursued?

I'm having to do manual schema customization more and more and I would love to have the "types will be generated based on the content model in Contentful" feature. Keep up the good work!

b-vb avatar Aug 08 '22 13:08 b-vb

Before we can give this PR to review to the Gatsby Core team, the following things needs to be resolved:

  • [x] Execute the https://gatsby.dev/node-convention-deprecation #37021
  • [x] Finally rewrite to TypeScript
  • [x] sys.type is the entity type (Entry, Asset), so it should not become sys.contentType
  • [x] Reverse references should be restructured to use linkedFrom like Contentful does
  • [x] https://github.com/gatsbyjs/gatsby/pull/30855#discussion_r850291289
  • [x] https://github.com/gatsbyjs/gatsby/pull/30855#pullrequestreview-942074399

Also, resolve the following todos:

/gatsby-source-contentful/src/__fixtures__/rich-text-data.js
  802,4: // @todo this fixture is unused

/gatsby-source-contentful/src/create-schema-customization.js
  169,6:   // @todo what do we do when preview is enabled? Emptry required fields are valid for Contentfuls CP-API
  516,6:   // @todo Is there a way to have this as string and let transformer-remark replace it with an object?
  523,10:       // @todo do we need a node interface here?

/gatsby-source-contentful/src/fetch.js
  59,8:      * @todo properly type this with TS

/gatsby-source-contentful/src/source-nodes.js
  340,14:           // @todo Detect reference fields based on schema. Should be easier to achieve in the upcoming version.
  493,12:         // @todo update the structure of tags

Open discussion points:

  • Automatically turning all long text fields into MD(X) able text nodes is performance intense, especially in big projects. https://github.com/gatsbyjs/gatsby/pull/30855#discussion_r613426059
  • The data schema of the Contentful Preview API differs from the Contentful Delivery API. E.G. some fields are not required. Should we just ignore the "required field value" flag in general? How does the Contentful GraphQL API deal with this?

axe312ger avatar Nov 09 '22 16:11 axe312ger

@b-vb organizational blockers are resolved. Back on track now :)

axe312ger avatar Nov 17 '22 10:11 axe312ger

Is it possible to get a status of this? It would make my day sooo much easier. The current implementation of schema generation is by far the most time consuming "feature" when working with Gatsby/Contentful :(

samuel99 avatar Jan 26 '23 08:01 samuel99

@samuel99 I know. It is a lot to do and we regularily have organizational blockers.

I won't give any estimation anymore, sorry.

We might get a alpha/beta as soon we get #37123 merged

axe312ger avatar Feb 06 '23 11:02 axe312ger

As many are waiting here for some progress:

You consider yourself good in TypeScript?

I am happy about ANY feedback in #37123 !

axe312ger avatar Mar 08 '23 08:03 axe312ger

Update:

Step by step, we are getting towards public alpha. Currently trying to please CI & e2e tests with the just merged TS rewrite.

Afterwards, I'll work on the bullet points in https://github.com/gatsbyjs/gatsby/pull/30855#issuecomment-1309013706.

In the meanwhile, the Gatsby team (WITHOUT exact date) is preparing a proper alpha/beta release process for this plugin so everyone can try and give feedback.

axe312ger avatar Mar 17 '23 10:03 axe312ger

Last tasks to solve for first alpha

Hopefully complete list 😇

  • [x] Finish and merge https://github.com/gatsbyjs/gatsby/pull/37811
  • [x] Refactor in upcoming changes from https://github.com/gatsbyjs/gatsby/pull/37910
  • [x] Double check that we run latest dependencies
  • [x] Check if Cypress 12 does screenshots properly
  • [x] Automatically turning all long text fields into MD(X) able text nodes is performance intense, especially in big projects. https://github.com/gatsbyjs/gatsby/pull/30855#discussion_r613426059
  • [x] The data schema of the Contentful Preview API differs from the Contentful Delivery API. E.G. some fields are not required. Should we just ignore the "required field value" flag in general? How does the Contentful GraphQL API deal with this?
  • [x] Implement Contentful JS SDK v10 with official TS support and remove our custom classes
  • [ ] Bug: Warm Builds reset localFile to null on already existing nodes
  • [x] Resolve all of the following ToDos:

./gatsby/packages/gatsby-source-contentful/src/__fixtures__/rich-text-data.js

  • [x] 802,4: // TODO: this fixture is unused

./gatsby/packages/gatsby-source-contentful/src/types/contentful.ts

  • [x] 60,6: // TODO: this field type might be defined by Gatsby already?
  • [x] 62,6: // TODO: this field type might be defined by Gatsby already?

./gatsby/packages/gatsby-source-contentful/src/create-schema-customization.ts

  • [x] 237,6: // TODO: what do we do when preview is enabled? Emptry required fields are valid for Contentfuls CP-API
  • [x] 465,12: // TODO: fix the type for extraArgs in gatsby-plugin-iomage so we dont have to cast to any here
  • [x] 639,8: // TODO: Is there a way to have this as string and let transformer-remark replace it with an object?
  • [x] 646,12: // TODO: do we need a node interface here?

./gatsby/packages/gatsby-source-contentful/src/fetch.ts

  • [x] 74,8: * TODO: properly type this with TS

./gatsby/packages/gatsby-source-contentful/src/normalize.ts

  • [x] 111,4: // TODO: space id is actually not factored in here!
  • [x] 685,12: // TODO:: how expensive is this?

./gatsby/packages/gatsby-source-contentful/src/source-nodes.ts

  • [x] 310,12: // TODO: nodes of text fields should be deleted as well
  • [x] 351,8: // TODO: mirror structure of Contentful GraphQL API, as it prevents field name overlaps
  • [x] 519,10: // TODO add batching in gatsby-core
  • [x] 566,14: // TODO: update the structure of tags

axe312ger avatar Apr 12 '23 10:04 axe312ger

As CTF JS v10 is now released, with full typescript support, we can remove a lot of type definitions from this branch! :)

https://github.com/contentful/contentful.js/releases/tag/v10.0.0

axe312ger avatar Apr 18 '23 07:04 axe312ger

As a reference for followers, still working on the bullet points in https://github.com/gatsbyjs/gatsby/pull/30855#issuecomment-1505013126

Mostly small things left :)

axe312ger avatar Jun 19 '23 09:06 axe312ger

Feature we should add:

A debugflag, that lets all data loaded from CTF APIs stored to local file system. That way, if issues arise, customers can share their API responses easily with CTF support.

axe312ger avatar Jul 07 '23 10:07 axe312ger

I am aware of one more bug, after fixing it, I'll test this version deeply with my personal Gatsby+Contentful projects.

Then we are ready for a public canary/alpha!

  • [x] Bug: Warm Builds reset localFile to null on already existing nodes

axe312ger avatar Jul 13 '23 11:07 axe312ger

Todos and bugs I found while testing with a real project:

  • [x] useNameForId is not taken into account for reference fields
  • [x] metadata should be called contentfulMetadata
  • [x] linkedFrom is missing from restricted field names
  • [ ] we should provide a way/tutorial on how to type JSON fields now
  • [x] Progress bar is broken
  • [x] codemods: node_locale needs to be transformed when filtering (all fields of sys field)
  • [x] codemods: asset fields flattening when filtering does not work
  • [x] codemods: metadata -> contentfulMetadata
  • [x] codemods: content type names should also be rewritten in fragments (fragment Foo on ContentfulFooBar)
  • [x] codemods: content type names should also be rewritten in query extends (... on ContentfulFooBar {)
  • [x] codemods: single value query filter does not transform id (contentful_id -> sys.id)
  • [x] codemods: sort is missing
  • [x] codemods: would be great if we could rename ContentTypeNames also in the config of custom field resolvers (createResolvers)
  • [x] codemods: warn users that have createSchemaCustomization implemented -> their code is either outdated or no more needed
  • [x] codemods: flatten asset structure (foo.file.details.width -> foo.width)
  • ... this list will be extended ...

axe312ger avatar Aug 15 '23 09:08 axe312ger

Next list of bugs I will fix before alpha:

  • [x] sys.contentType is always null
  • [x] fix foreignReferenceMap so big projects can store to gatsby cache
  • [x] unions should not point to no more existing content types (validations in CTF can contain this data)
  • [x] applying codemods on huge projects results in a OOM error (or requires 10GB RAM)
    • NODE_OPTIONS=--max_old_space_size=10240 npx jscodeshift -t ./packages/gatsby-codemods/transforms/gatsby-source-contentful.js src/**/*.ts src/**/*.tsx ./*.ts ./*.js
  • [x] fix integration with gatsby-plugin-page-creator
  • ... to be extended ...

axe312ger avatar Sep 04 '23 11:09 axe312ger

We finally released a public alpha :)

Please try it out and give feedback in the new discussion -> https://github.com/gatsbyjs/gatsby/discussions/38585

axe312ger avatar Sep 28 '23 10:09 axe312ger

Published new alpha version - [email protected]

pieh avatar Dec 21 '23 17:12 pieh

Published new alpha version - [email protected]

axe312ger avatar Jan 10 '24 13:01 axe312ger

We published new alpha versions:

axe312ger avatar Mar 08 '24 15:03 axe312ger

👋 Long time plugin user here, nice progress!

Just to add context on the necessity of getting this PR merged: I started a discussion on the potential workarounds I need to do to get a site working that has a restricted content model ID, where I'm confident that this PR could solve my problems in a better way than the alternatives I can think of. https://github.com/gatsbyjs/gatsby/discussions/38908

bartveneman avatar Mar 22 '24 17:03 bartveneman