Skip to content
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(contentful): properly delete deleted entries and assets #5756

Merged
merged 2 commits into from
Jun 13, 2018

Conversation

axe312ger
Copy link
Collaborator

fixes #5747

This should ensure that the correct id's are passed to deleteNode

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jun 6, 2018

Deploy preview for gatsbygram ready!

Built with commit b23923a

https://deploy-preview-5756--gatsbygram.netlify.com

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jun 6, 2018

Deploy preview for using-drupal ready!

Built with commit b23923a

https://deploy-preview-5756--using-drupal.netlify.com

@@ -75,12 +75,17 @@ exports.sourceNodes = async (
// Remove deleted entries & assets.
// TODO figure out if entries referencing now deleted entries/assets
// are "updated" so will get the now deleted reference removed.

function deleteContentfulNode (node) {
const id = normalize.fixId(node.sys.id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't normalize.fixId already done here - https://github.com/axe312ger/gatsby/blob/3058f95dc020524dec1242da3523a1e9a5f73e5d/packages/gatsby-source-contentful/src/fetch.js#L75-L86 (fixIds seem to traverse passed object and run fixId on id fields) ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the id is passed to your new function not the node. You need to fix the id and then use that to get the node.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node is passed there - lines that maps node -> id are deleted ( lines 79, 82 ), but I checked again and we can skip normalizing id here (it's already normalized)

@pieh
Copy link
Contributor

pieh commented Jun 12, 2018

Tested it locally and this now properly deletes nodes for deleted entries of non-default locales. But I couldn't replicate exact issue presented in #5747 (nodes with null fields). I would see nodes that should be deleted but containing all data before applying changes from this PR.

@axe312ger
Copy link
Collaborator Author

I removed the normalize.fixID as suggested.

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank You!

@pieh pieh merged commit cd9ae49 into gatsbyjs:master Jun 13, 2018
m-allanson added a commit that referenced this pull request Jun 14, 2018
* master: (26 commits)
  Publish
  [feat] Extract internal plugin for automatically creating pages so we can reuse for other directories (#4490)
  fix(contentful): properly delete deleted entries and assets (#5756)
  Fix typo
  add Linda's post (#5817)
  [gatsby-source-contentful] Fix prepareJSONNode for array normalization (#5107)
  Process Contentful images inlined in markdown (#5542)
  feat(contentful): add traced SVGs to Contentful images (#5659)
  [SQIP] Add documentation (#5287)
  Add site to showcase (#5819)
  [gatsby-remark-copy-linked-files] Support reference-style images (#5818)
  Update docs for gatsby-plugin-catch-links (#5843)
  Added ⋅ representing a space to sub-list examples to reflect actual code (#5829)
  Adding tigerfacilityservices.com to showcase (#5855)
  fix schema type conflict reporter regressions (#5805)
  [gatsby-plugin-feed] Support nested output directory (#5820)
  Make cache available to the plugins (#5840)
  [gatsby-source-contentful] update data and jest snapshots (#5802)
  [gatsby-plugin-manifest] Replace all manifest.json with manifest.webmanifest in comments/docs (#5157)
  Update deploy-gatsby.md (#5800)
  ...

# Conflicts:
#	docs/tutorial/part-one/index.md
#	examples/using-sqip/src/components/polaroid.js
#	packages/gatsby-image/README.md
#	packages/gatsby-image/package.json
#	packages/gatsby-plugin-catch-links/package.json
#	packages/gatsby-plugin-feed/package.json
#	packages/gatsby-plugin-manifest/package.json
#	packages/gatsby-plugin-page-creator/src/gatsby-node.js
#	packages/gatsby-remark-copy-linked-files/package.json
#	packages/gatsby-source-contentful/package.json
#	packages/gatsby-source-contentful/src/__tests__/__snapshots__/normalize.js.snap
#	packages/gatsby-source-contentful/src/extend-node-type.js
#	packages/gatsby-source-contentful/src/gatsby-node.js
#	packages/gatsby-source-contentful/src/normalize.js
#	packages/gatsby-transformer-remark/package.json
#	packages/gatsby-transformer-remark/src/__tests__/__snapshots__/gatsby-node.js.snap
#	packages/gatsby-transformer-sqip/package.json
#	packages/gatsby-transformer-sqip/src/extend-node-type.js
#	packages/gatsby/package.json
#	packages/gatsby/src/bootstrap/load-plugins/__tests__/__snapshots__/load-plugins.js.snap
#	packages/gatsby/src/bootstrap/load-plugins/load.js
#	www/src/data/sites.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[source-contentful] node deletions bug
4 participants