contentful / contentful.js

JavaScript library for Contentful's Delivery API (node & browser)
https://contentful.github.io/contentful.js
MIT License
1.2k stars 200 forks source link

Add an option to avoid circular references in the object returned by `getEntries()` #377

Closed jtomaszewski closed 2 years ago

jtomaszewski commented 4 years ago

Currently I'm loading page's data (from Contentful) in a next.js getStaticProps function. After that, I'm returning that data as a prop to my react page component.

However, getStaticProps function must not return JSON with circular references. If you do that, you end up with "circular references cannot be expressed in JSON" error.

To go around it I had to call .stringifySafe() on the entries and then parse that stringified string, so I could end up with the same JSON object, but without the circular references in it:

function removeEntryCollectionCircularReferences<T>(
  entries: EntryCollection<T>
): EntryCollection<T> {
  return JSON.parse(entries.stringifySafe()) as typeof entries;
}

export const getStaticProps: GetStaticProps = async context => {
  const pages = await contentfulClient.getEntries<IPageFields>({
    content_type: 'page',
    'fields.urlSlug': context.params.id
  });
  const page = removeEntryCollectionCircularReferences(pages).items[0];

  return { props: { page } };
};

However it is not ideal, as I'm stringifying and then parsing that whole JSON needlessly, also it requires me to have that util function. (I suspect more people will end up with similar problem soon.)

Ideally, maybe we could tell the Contentful Client not to fully serialize the linked entry, if it has been already included in the tree above it? It could be configured with a resolveCircularReferences: boolean option passed to the client or getEntries() method.

What do you think?

Environment

paulobmarcos commented 4 years ago

Same problem here. It's a strange behavior, or so it seems.

Imagine that we have the following:

A ---references---> B ---references---> A

Current Behavior:

Even if you use the include parameter to limit the depth of resolved links, it will still return the last entry A fully resolved instead of a link.

Expected Behavior:

It would respect the include parameter and return the last A entry as a link.

Note

I have seen multiple issues open about this problem, even closed ones from a few years ago. Is there any expectation to tackle this problem?

Originally posted by @paulobmarcos in https://github.com/contentful/contentful.js/issues/297#issuecomment-601924392

paulobmarcos commented 4 years ago

After checking who was responsible for transforming the result, I found it to be : contentful-resolve-response.

Maybe that package should accept an option to go deep until a certain level.

In most cases, that deep level is the include parameter value. Although an option like resolveLinksDepth could be very useful to avoid circular references from a certain point.

bryandowning commented 4 years ago

I just ran into this problem as well. In case anyone else lands here who also happens to be using normalizr, the fix for this is SUPER simple — just update normalizr to at least 3.4.0 (3.5.0 would be better as performance improvements were introduced there).

normalizr supports normalizing circular references without you needing to do anything outside of defining your schemas and normalizing the contentful responses.

patrickedqvist commented 4 years ago

I've tried @bryandowning approach using NextJS but without a redux implementation, it grows quite complex. You need some sort of global state management to access the output from normalize.

I have the following content structure:

Page: Page A ---references---> Sections ---references---> Call To Action ---references---> Page: Page B

Page B contains a field parentPage which is a reference to Page A. Overall my experience with Contentful has been great but creating hierarchical content with references is a pain and not something I have been able to find a good solution too when creating websites.

I will probably look into a solution that resolves Call To Action references to Pages so that it does not include the Sections reference field which is causing the circular reference.

bryandowning commented 4 years ago

@patrickedqvist You really shouldn't need to resolve/flatten those Call To Action references, and doing it is a slippery slope. As long as you have a schema defined for Page, Sections, and Call To Action, AND you have all the fields pointing to the proper schemas — you should be good to go. However, setting all that up is a bit tricky. Here's some (incomplete) sample code that may help — I don't think it maps exactly to your setup, but it's close and it communicates the idea:

import { lensPath, view } from 'ramda'
import { schema } from 'normalizr'

// All of your section schemas get defined here
export const callToActionSchema = new schema.Entity('ctas')

// Create an object of all available section schemas
export const sectionSchemas = {
  callToAction: callToActionSchema,
}

// This is used to tell normalizr where to find the content type (using some helpers from ramda)
// The value in sys.contentType.sys.id needs to match an object key in sectionSchemas
export const lensEntityType = lensPath(['sys', 'contentType', 'sys', 'id'])
export function getEntityType(entity) {
  return view(lensEntityType, entity)
}

// Use schema.Union to allow many different schemas to be used for a "section"
export const sectionSchema = new schema.Union(sectionSchemas, getEntityType)

// Make your pages reference sections
export const pageSchema = new schema.Entity('pages', {
  sections: [sectionSchema],
})

// This is the tricky bit - use the define method to add fields to schemas after
// they have been declared. This is key to avoid circular reference issues with
// your code imports.
pageSchema.define({ parentPage: pageSchema })
callToActionSchema.define({ page: pageSchema })
patrickedqvist commented 4 years ago

@bryandowning Thank you for your answer and valuable code example. I did try with something close to what you have shown. The problem is however with storing the results from the normalization in a way so it is easily accessible from a nested component. Now I can be wrong but from what I remember of using NextJS and next-redux-wrapper for storing data is that you couldn't use it and also use static generation (sort of the scope of this discussion but it is a real-life example). Maybe that has changed.

Here is an example of what I believe I would need for accessing the results from a possibly nested component.

// CallToAction.tsx

interface Props {
  reference: string // The id we have from the normalizr result
  label: string
}

const LinkToPage = ({ reference, label )} => {

  const pages = useSelector(state => state.pages)
  const page = useSelector(state => state.pages[reference]);

  // Check if page has a parentPage and if that page also has a parent page and so on. 
  // Returns an array of strings. e.g ['page-a', 'page-b']
  const parentPages = resolveParentPageHierarchy(page, pages)

  const finalSlug = parentPages.length > 0 ? parentPages.join('/') + '/' + page.slug : page.slug

  return (
   <a href={finalSlug}>
      {label}
    </a>
  )
}
// Example - In a section
<div>
  <LinkToPage reference={section.callToAction} label={'Read More'} />
</div>
bryandowning commented 4 years ago

@patrickedqvist I'm not aware of any reason static generation in Next wouldn't be possible when using some kind of global state management like Redux, but this may depend on your version of Next.

More info: https://nextjs.org/docs/basic-features/data-fetching#getstaticprops-static-generation

But also check out this recent Syntax episode about state management — there are some really great alternatives to Redux: https://syntax.fm/show/272/react-state-round-up

Side note: I would argue that if you need to use normalizr, you should definitely be using some kind of global state management. I can't think of a scenario where you'd want to normalize without state management.

patrickedqvist commented 4 years ago

@bryandowning I've reread the documentation for next-redux-wrapper again, and it does no longer seem to force you to opt-out of static generation, however, if you use for example Redux-Saga then you must opt-out - "Keep in mind that this setup will opt you out of Automatic Static Optimization: opt-out-auto-static-optimization".

Redux saga has been my go-to package for handling side effects with Redux, and while it is not a requirement, it is very helpful. See usage with redux saga for the documentation.

I agree with your side note.

bryandowning commented 4 years ago

Interesting, thanks for pointing to that. However, worth noting that you're only opted out if you use getInitialProps in _app.js, which is not required for redux-saga:

https://github.com/kirill-konshin/next-redux-wrapper#usage-without-getinitialprops-inside-_app

patrickedqvist commented 4 years ago

@bryandowning Oh, that was something I had missed. I will have to look into it and try it out.

DanielOrtel commented 3 years ago

any word on this? We use immutableJS and store the data returned by getEntries in Redux, but immutable's fromJS breaks on circular dependencies completely. Sadly, there's no way atm to avoid immutable, so it'd be nice to have the option to make getEntries not circularly dependent.

juanbiberretta commented 3 years ago

Same here, I'd love for this to be at least looked at... We have to run out responses through a cycle remover on Gatsby otherwise it fails when trying to create pages.

DerekLoop commented 3 years ago

Bump? 😊 Circular references would seem common in any blog, such as if one blog post has an embedded entry referencing another blog post...

edit: I was able to fix my use case by using the .stringifySafe() helper method:

before

  async fetchEntry() {
    const client = createClient({
      space: 000, // spaceID
      accessToken: 000, // accessToken
    })
    const items = await client.getEntries({
      content_type: 000, // entryID
      include: 4, // how many levels to include
    })
    return items
  }

after

  async fetchEntry() {
    const client = createClient({
      space: 000, // spaceID
      accessToken: 000, // accessToken
    })
    const items = await client.getEntries({
      content_type: 000, // entryID
      include: 4, // how many levels to include
    })
    return JSON.parse(items.stringifySafe())
  }
maapteh commented 2 years ago

https://raw.githubusercontent.com/contentful/contentful-resolve-response/master/README.md the lib states: Circular references are possible, still!!

This is the first CMS where i will be very happy when the client ditches it. Who wants to use ten of their libs and still finding out it creates circulair dependencies which you then have to hack with a non performant eventloop blocking way, which then leaves out data...). So on a FAQ pointing to another link on same page, you can't.

Even when doing this blocking hack mentioned above, you loose the fields you need for your display, instead it will add circular: true to your object, handy. Also the stringification and parsing can totally freeze your container when you have a big response (homepage) :(

link: {
    sys: {
      type: 'Link',
      linkType: 'Entry',
      id: '7pcyxA0H5o2RZ7yCq6nmGM',
      circular: true
    }
  }

As storage all this entries stuff it's modelling is very powerful, but as a consumer for its api i'm only interested in its flattened response which takes care of all this. I rather have the same content twice then this total crap. I have to tell my editors well you can't do this. They will laugh. This model showing all entries is more a backend driven model, as a consumer im just interested in the result of the fields for a page/component, give me its data not fields inside fields which refer to some entry.

And then keeping these kind of issue open for years is not what you expect from a paid solution ....

If you use NextJS/Gatsby/etc and you have a real content driven application, think twice about using this CMS in the first place ...

(active development during 2016/2017, then around 2020 not really anymore, it seems some focus shift on GQL). Hey wait thats perfect ... also not .... the complexity for a normal e-commerce homepage is too big by testing a simple limited page query, i can only guess but i will never end up in writing ten queries (freeText json is not enough, you have to take stuff from links, then if you place it in multiple components like FAQ or FreeText then the schema for it thinks they are two different entities while they are the same etc etc etc) for one page retrieval.

I hope you will make the product better for your co-developers. Best wishes!

ps i found the cullprit, its in freeText component, a link inside it can link to a page and take alle data from that which links again to that page. Im going to patch the freeText component in my service so i will not need this stringify. The parse of stringified is really event blocking and should never be used!

maapteh commented 2 years ago

Yes, so my culprit was everywhere where i used FreeText component. To fix this circular there, for the 'entry-hyperlink' im now traversing the content[] and patching these to only include the title, link/slug and pass these back instead of a possible whole data model of the page it links to with all that content!!! etc etc. Hope this helps others!

So i didnt use any of the client sdk helpers! No stringification needed :) The flatten response code is only using the contentful-resolve-response. So im using own fetch and with the first result im flattening like:

import resolveResponse from 'contentful-resolve-response';

// reason: any data can be given, contentfull raw responses are not typed
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export const flattenResponse = (data: any) => {
  // setup is simplified and taken from: https://github.com/contentful/contentful.js/blob/47d9d0398960b58627659e2d99ca27aa1dae8871/lib/entities/entry.js#L100

  const dataCopy = typeof data === 'object' ? { ...data } : data;

  return resolveResponse(dataCopy, {
    removeUnresolved: true,
    itemEntryPoints: ['fields'],
  });
};

Then that result im traversing and looking for FreeText parts and in these im fixing to give data target only three fields back, and these three fields i pickyup in the FreeText React renderer.

andreascful commented 2 years ago

Thank you @maapteh for providing your solution. I'm closing this issue for now since it seems that there are solutions and/or workarounds for this issue given in the thread. If anything pops up again we can definitely open it up again.