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

chore(*): Use new createContentDigest helper #8992

Merged
merged 70 commits into from
Apr 23, 2019
Merged

chore(*): Use new createContentDigest helper #8992

merged 70 commits into from
Apr 23, 2019

Conversation

samscha
Copy link
Contributor

@samscha samscha commented Oct 10, 2018

Note: this is my first pr to Gatsby so if I'm missing something (best practices included), please do tell me about them and I'll update this/future pr's.

Added createContentDigest helper for gatsby-transformer-remark and gatsby-source-filesystem.

I opened this pr right now mainly because I wanted to make sure I was adding the createContentDigest helper to these two plugins correctly and not breaking anything (before going on to other plugins).

If it's more convenient, and I have verification I correctly added the createContentDigest helper to these two plugins, this pr can be closed and I can open a new pr later with more plugins that have the createContentDigest helper added.

Thanks!

from
#8805

@samscha samscha requested a review from a team October 10, 2018 14:07
Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

This looks good! I've seen a few PRs now where createContentDigest is mocked in tests, which is technically fine but I think we'd prefer if the real implementation was made available to nodes so snapshots track any regressions there.

Sound like work you're interested in fixing up?

@samscha
Copy link
Contributor Author

samscha commented Oct 10, 2018

Yes I can definitely work on re-implementing tests to use createContentDigest.

For clarification, this would mean that the snapshots, instead of having contentDigest, would have their unique md5 hash?

Thanks!

@DSchau
Copy link
Contributor

DSchau commented Oct 10, 2018

@samscha that is exactly right!

@samscha samscha requested a review from a team as a code owner October 15, 2018 17:00
@samscha samscha requested a review from a team October 15, 2018 17:00
@samscha samscha requested a review from a team as a code owner October 15, 2018 17:00
const { resolve, parse } = require(`path`)

const axios = require(`axios`)
const { pathExists, createWriteStream } = require(`fs-extra`)

const createContentDigest = require(`../../gatsby/src/utils/create-content-digest`)
Copy link
Contributor

Choose a reason for hiding this comment

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

we should pass this from caller and not import directly here

Copy link
Contributor

Choose a reason for hiding this comment

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

we should only import it directly for tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you for the comment! getting from the caller makes a lot more sense! I fixed all but two of these instances in previous commits.

I'm going to work on removing the import from gatsby-source-contentful and gatsby-transformer-sqip (both use the cache-image code mentioned above)

@wardpeet wardpeet changed the title add createContentDigest helper for gatsby-transformer-remark and gatsby-source-filesystem chore(*): Use new createContentDigest helper Apr 23, 2019
@wardpeet wardpeet removed their assignment Apr 23, 2019
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Looks good to me! I like const { createContentDigest } = require(gatsby/utils) we probably want to upgrade all code when we're done migrating

@wardpeet wardpeet added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Apr 23, 2019
@KyleAMathews KyleAMathews merged commit c91d110 into gatsbyjs:master Apr 23, 2019
@gatsbot
Copy link

gatsbot bot commented Apr 23, 2019

Holy buckets, @samscha — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

@KyleAMathews
Copy link
Contributor

Thanks!

@KyleAMathews
Copy link
Contributor

Hey @samscha! We unfortunately had to revert this — looks like there were a couple of problems that we didn't catch before merging. Could you put up a new PR with these fixes?

Basically we need to keep the old implementations around as a fallback as many people won't upgrade gatsby when upgrading/installing a package so the utils.js file wouldn't be there. Also, currently that file is blocked from being published so that'd need fixed here https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/package.json#L147-L152

For the fallbacks, perhaps move them to a separate file e.g. fallbacks.js and add a TODO at the top to remove them in gatsby v3?

@samscha
Copy link
Contributor Author

samscha commented Apr 24, 2019

Got it. Will open up a new PR!

Where exactly would the fallbacks.js live?

@KyleAMathews
Copy link
Contributor

Thanks!

The fallback.js file would be in the src directory for each plugin. It would try to require the core code and if that fails, use the fallback code. This way when v3 comes, all we need to do is to remove the fallback.js file and replace any code importing code from it to just use the core code.

mwfrost pushed a commit to mwfrost/gatsby that referenced this pull request Apr 20, 2023
* add createContentDigest helper for gatsby-transformer-remark

* add createContentDigest helper for gatsby-source-filesystem

* Added createContentDigest in test

* Added missing createContentDigest param

* update createContentDigest helper tests for gatsby-transformer-remark

* update createContentDigest helper tests for gatsby-source-filesystem

* update createContentDigest helper tests for gatsby-transformer-csv

* add createContentDigest helper for internal-plugins/internal-data-bridge

* add createContentDigest helper for gatsby-source-hacker-news

* add createContentDigest helper for gatsby-transformer-excel

* add createContentDigest helper for gatsby-source-mongodb

* add createContentDigest helper for gatsby-source-lever

* add createContentDigest helper for internal-plugins/query-runner

* Updated gatsby dep

gatsby-source-hacker-news
gatsby-source-lever
gatsby-source-mongodb
gatsby-transformer-excel

* add createContentDigest helper for gatsby-source-contentful

* add createContentDigest helper for gatsby-transformer-hjson

* add createContentDigest helper for gatsby-transformer-sqip

* add createContentDigest helper for gatsby-source-npm-package-search

* add createContentDigest helper for gatsby-transformer-toml

* update createContentDigest helper for gatsby-source-mongodb

remove direct import

* update createContentDigest helper for gatsby-source-lever

could not find import for createGraphQLNode, added helper anyways

* update createContentDigest helper for internal-plugins/query-runner

modify src/bootstrap (imports writeRedirects)

* update createContentDigest helper for gatsby-source-npm-package-search

* update createContentDigest helper for gatsby-source-contentful*

TODO add ccd (createContentDigest) in cache-image

* update createContentDigest helper for gatsby-transformer-sqip*

TODO add ccd (createContentDigest) in generate-sqip

* update createContentDigest helper for gatsby-transformer-sqip

removed ccd from tests

* use caret version selector

* no need to import it - it's passed to `sourceNodes`

* merge again

* update to pass tests

* update hash to use createContentDigest

* move createContentDigest to import

* move createContentDigest to import for gatsby-source-contentful

* remove createContentDigest import in gatsby-source-filesystem test

* rereplace createContentDigest with crypto in gatsby-source-filesystem

* revert createContentDigest to crypto for create-file-node in gatsby-source-filesystem

* revert createContentDigest for gatsby-plugin-sharp

* update crypto misspell

* revert gatsby-remark-contentful and gatsby-source-contentful

* fix old code

* fix old code v2

* update gatsby pkg

* revert gatsby-transformer-remark

* add export for gatsby utils

* update gatsby-source-filesystem

* revert gatsby-source-graphql

* revert gatsby-source-graphql

* revert gatsby-source-hacker-news

* revert gatsby-source-lever

* revert gatsby-source-medium

* revert gatsby-source-mongodb

* revert gatsby-source-mongodb mapping

* revert gatsby-source-npm-package-search

* revert gatsby-source-wikipedia

* revert gatsby-source-wordpress

* revert gatsby-transformer-csv

* revert gatsby-transformer-documentationjs

* revert gatsby-transformer-excel

* revert gatsby-transformer-hjson

* revert other packages

* revert some pkgs

* add gatsby

* move gatsby to peerdeps
mwfrost pushed a commit to mwfrost/gatsby that referenced this pull request Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants