feat: update gatsby plugins to use new createContentDigest helper

DSchau commented 5 years ago

Summary

We recently added a createContentDigest helper which is exposed to any gatsby plugin. This internalizes the unique key creation, which changes based on changing content.

We've added this to several packages, and we should now add it to any/all packages.

Basic example

Any plugin that implements the following code snippet (or close to it) can be refactored with this new helper:

const crypto = require(`crypto`)

module.exports = ({ createNode }) => {
  const obj = { whatever: true }

  // this is the key one!
  const contentDigest = crypto
  .createHash(`md5`)
  .update(JSON.stringify(obj))
  .digest(`hex`)

  createNode() // use content digest
}

can be replaced with

const { createContentDigest } = require('gatsby-core-utils');

module.exports = ({ createNode }) => {
  const obj = { whatever: true }

  // this is the key one!
  const contentDigest = createContentDigest(obj)

  createNode() // use content digest
}

Motivation

Cleaning up code and keeping it DRY 💪

Additional Info

Any plugin that implements this new API should also bump it's peerDependency on gatsby, e.g.

{
  "peerDependencies": {
    "gatsby": ">2.0.15"
  }
}

samscha commented 5 years ago

Greetings,

I would like to start contributing to Gatsby by helping with this feature / enhancement.

For clarification,

I was going through the published plugins library, and identified the unique key creation pattern stated above in the gatsby-source-filesystem plugin:

src/create-file-node.js

if (stats.isDirectory()) {
  const contentDigest = crypto
    .createHash(`md5`)
    .update(
      JSON.stringify({ stats: stats, absolutePath: slashedFile.absolutePath })
    )
    .digest(`hex`)

An example enhanced refactor could be:

exports.createFileNode = async (
  pathToFile,
  createContentDigest, // <-- add this
  createNodeId,
  pluginOptions = {}
) => {
  // other code

  if (stats.isDirectory()) {
    const obj = { stats: stats, absolutePath: slashedFile.absolutePath };

    const contentDigest = createContentDigest(obj)
                       // createContentDigest({ stats: stats, ... });

However,

src/create-remote-file-node.js

// line 223
const fileNode =
  await createFileNode(
    filename,
    createContentDigest, // <-- where would this come from? ****
    createNodeId,
    {}
  );

I'm not sure how to pass createContentDigest as a parameter nor where it comes from (is it the "main" repo?). The same situation is found in src/gatsby-node and in general, any file that imports create-file-node.js will need to pass this createContentDigest. I'll take a look around though and try to find it.

I wanted to make sure I understood the enhancement before requesting it!

Thanks!

Sam

P.S. I'm definitely going to rebuild some of my hobby projects with Gatsby!

samscha commented 5 years ago

Thank you for the reference. I'll do some testing on my own for some of the most popular plugins and report back / open a pr!

amberleyromo commented 5 years ago

@DSchau Maybe it would be useful to break this apart into different issues for the different plugins? Or at least a listing of todos in this issue description?

gatsbot[bot] commented 5 years ago

Old issues will be closed after 30 days of inactivity. This issue has been quiet for 20 days and is being marked as stale. Reply here or add the label "not stale" to keep this issue open!

frinyvonnick commented 5 years ago

Here is the list of plugins that seems not using createContentDigest as suggested @amberleyromo :

[x] gatsby-source-hacker-news(#13000)
[x] gatsby-transformer-excel(#13000)
[x] gatsby-source-wikipedia(#13000)
[x] gatsby-source-mongodb 1, 2 (#13293)
[x] gatsby-source-filesystem 1, 2(#8992)
[x] gatsby-source-contentful 1, 2 (#20239)
[x] gatsby-transformer-hjson(#13000)
[x] gatsby-transformer-sqip(#18534)
[x] gatsby-source-npm-package-search(#13000)
[x] gatsby-transformer-toml(#12973)
[x] gatsby-transformer-remark(#8992)
[x] gatsby-source-lever(#13000)
[x] gatsby-transformer-javascript-static-exports(#13000)
[x] gatsby-source-graphql(#13000)
[x] gatsby-source-medium(#12973)
[x] gatsby-transformer-react-docgen(#13000)
[x] gatsby-transformer-javascript-frontmatter(#13000)
[x] gatsby-transformer-screenshot(#18830)
[x] gatsby-remark-images-contentful(#14522)
[x] gatsby-source-drupal(#12973)
[x] gatsby-plugin-sharp
- src/process-file.js (#18139)
- src/index.js (#20076)
[x] gatsby-source-wordpress(#13000)
[x] gatsby-theme-notes(#20154)
[x] gatsby-plugin-preload-fonts (#20155)
[x] gatsby-plugin-mdx (#20239)
[x] gatsby-theme-blog-core (#20239)
[ ] gatsby-telemetry
- src/repository-id sha256
- src/tests/repository-id.js sha256
[ ] gatsby

I didn't list occurences in gatsby package. Could someone confirm this list ?

I could help to fix some of them 😄

wardpeet commented 5 years ago

@frinyvonnick feel free to already start on them ;) no need to wait for validation

frinyvonnick commented 5 years ago

@DSchau I don't know how to handle it in a setFieldsOnGraphQLNodeType function like in gatsby-transformer-sqip. Do you have any advices ?

gmertk commented 5 years ago

Hi gatsby folks, I'm jumping in to fix the remaining ones here.

gatsby-source-contentful has 2 references to crypto: 1- cache-image.js which is called by extend-node-type.js used as setFieldsOnGraphQLNodeType. However, setFieldsOnGraphQLNodeType doesn’t have createContentDigest as sourceNodes does. 2 - normalize.js which is called by sourceNodes. Fixing this one in #14567.
gatsby-transformer-sqip is similar to 1st issue of gatsby-source-contentful. crypto is used within setFieldsOnGraphQLNodeType.
gatsby-transformer-screenshot already uses createContentDigest here However its lambda function still uses crypto. That is ok I guess, as it isn’t a plugin?

How should we handle this in setFieldsOnGraphQLNodeType?

wardpeet commented 5 years ago

Sorry for keeping you waiting.

we want to hold off gatsby-source-contentful changes for now, as we have some big PRs open.

transformer-sqip should be able to use createContentDigest as it's passed down as args so it would be nice to use that one.

In gatsby-transformer-screenshot should use crypto as we don't want any deps on that function.

jbutz commented 4 years ago

There hasn't been any recent activity here, so I am going to try and jump in and take care of gastby-transformer-sqip and gastby-plugin-sharp.

muescha commented 4 years ago

Hi @DSchau can you please update the example in your issue with this hint:

https://github.com/gatsbyjs/gatsby/pull/18139#pullrequestreview-299211154 from @wardpeet

Hey @jbutz, thanks for the effort on this one. We currently have a more simplified snippet to replace contentDigest.

You can do the following
import { createContentDigest } from 'gatsby-core-utils';

DSchau commented 4 years ago

@muescha sure. done!

muescha commented 4 years ago

👍🏻

muescha commented 4 years ago

@DSchau

the implementation is now simplified by importing from gatsby-core-utils

Q: is it ok also refactor the "old style" usage to the simplified usage? so that always the code is the same?

muescha commented 4 years ago

i found this dead code:

src/utils/get-hash-fn.js

it is an not used function getHashFn -> can this be removed?

opened PR #20245 refactor(chore): remove unused function getHashFn

kindavishal commented 4 years ago

Thanks for the great session @sidharthachatterjee :sparkling_heart: But for some reason I am unable to switch branches. The following command was suppose to do the job git fetch origin && git checkout fix/content-digest but it's not fetching the other branches, only two from remote:

  remotes/origin/HEAD -> origin/master
  remotes/origin/master

My remote is set to this project's url, I however tried forking it to my account and then cloning it, but forking forks only the master branch, so no luck with that either! :/

muescha commented 4 years ago

@kindavishal maybe the PR #20239 is related

kindavishal commented 4 years ago

Maybe, I am unable to figure out why this is happening! Even if the PR is related, I should be able to switch branches right?

muescha commented 4 years ago

you are right,

but your question not relates to the title of the current issue, thats why i fear your question get lost here.

Sithartha mentioned in the linked PR the session with you, and the branch name is also fix/content-digest in the linked PR, so i guess the other issue is somehow better for asking.

kindavishal commented 4 years ago

Oh okay! Yes, that makes sense! I'll carry future conversations over there :slightly_smiling_face:

muescha commented 4 years ago

@sidharthachatterjee i updated the list in https://github.com/gatsbyjs/gatsby/issues/8805#issuecomment-477590874

but there are still left some places for the createContentDigest: in gatsby-telemetry and gatsby itself also

gatsbyjs / gatsby