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

feat(gatsby-source-filesystem): keep original name of remote files along with hash #9777

Merged
merged 6 commits into from
Dec 10, 2018

Conversation

hbish
Copy link
Contributor

@hbish hbish commented Nov 7, 2018

This PR is to enhance createRemoteFileNode to retain the filename of the remote file. Additional details in #9539 and prismicio/prismic-gatsby#40

cc: @DSchau sorry I messed up the other PR #9601, had to raise another.

@LekoArts
Copy link
Contributor

LekoArts commented Nov 7, 2018

Before merging this should be discussed:
#9539 (comment)

@LekoArts
Copy link
Contributor

@hbish Do you have time to make the changes to the PR? Namely putting the hash into a folder and the file into the folder as @pieh suggested in the issue?

@hbish
Copy link
Contributor Author

hbish commented Nov 23, 2018

@LekoArts I got back from holiday earlier this week. Just made the change. Please review.

LekoArts
LekoArts previously approved these changes Nov 23, 2018
Copy link
Contributor

@LekoArts LekoArts left a comment

Choose a reason for hiding this comment

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

LGTM

@LekoArts
Copy link
Contributor

Letting @DSchau have a look at it, too 👍

@LekoArts
Copy link
Contributor

LekoArts commented Dec 4, 2018

@DSchau @pieh BUMP ^

@jkjustjoshing
Copy link
Contributor

I've needed to put off releasing the product I'm working on awaiting this functionality. Any update on when it'll be able to get reviewed and added to a new version @DSchau @pieh?

@pieh
Copy link
Contributor

pieh commented Dec 10, 2018

@jkjustjoshing do You use publicURL to get link to your files? If so there will be additional change needed in here:

const fileName = `${file.name}-${file.internal.contentDigest}${
details.ext
}`

@pieh pieh self-assigned this Dec 10, 2018
@pieh
Copy link
Contributor

pieh commented Dec 10, 2018

There is some path problem with this:

/Users/misiek/dev/gatsby/examples/using-drupal/5bdb23fc01e4c72c3a39c6f1890ff2c3/.cache/gatsby-source-filesystem/bacon-themed_dinner.jpg

digest should be inside .cache/gatsby-source-filesystem, right now it creates multiple extra directories in project root dir

@pieh
Copy link
Contributor

pieh commented Dec 10, 2018

I'll push fix for this to get this merged

@LekoArts LekoArts dismissed their stale review December 10, 2018 17:32

Issues need to be solved

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.

Thanks @hbish!

@pieh pieh merged commit dfc069d into gatsbyjs:master Dec 10, 2018
@gatsbot
Copy link

gatsbot bot commented Dec 10, 2018

Holy buckets, @hbish — 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!

m-allanson added a commit to Bouncey/gatsby that referenced this pull request Dec 10, 2018
* master: (35 commits)
  feat(gatsby-source-filesystem): keep original name of remote files (gatsbyjs#9777)
  docs(gatsby-source-contentful): Rewrite of gatsby-source-contentful query section (gatsbyjs#7533)
  Update "Deploying with Now" Guide (gatsbyjs#10390)
  Add Matomo to list of analytics plugins (gatsbyjs#10372)
  Add satispay.com to showcase (gatsbyjs#10380)
  Adds @goblindegook/gatsby-starter-typescript (gatsbyjs#10377)
  Fix typo in gatsby-remark-code-repls sample `gatsby-config.json` in README (gatsbyjs#10361)
  fix(gatsby): fix false type conflict warning on plugin fields (gatsbyjs#10355)
  fix(docs): Just a small typo fix in the docs (gatsbyjs#10359)
  feat(gatsby-image): add onStartLoad prop  (gatsbyjs#6702)
  fix(docs): add Ecosystem to docs sidebar, consistency with tutorial sidebar (gatsbyjs#10350)
  fix(www): Starters.yaml housekeeping (gatsbyjs#10354)
  docs: add ttag starter (gatsbyjs#10352)
  docs: document branching (gatsbyjs#9983)
  plugin checker initial commit (gatsbyjs#7062)
  docs: new starter features is required (gatsbyjs#10353)
  docs: migrated line highlighting to highlight-line, highlight-start, highlight-end (gatsbyjs#10202)
  Add Birra Napoli to site showcase (gatsbyjs#10344)
  Add BetterDocs to site showcase (gatsbyjs#10349)
  chore(release): Publish
  ...
m-allanson added a commit to markza/gatsby that referenced this pull request Dec 11, 2018
* master: (1432 commits)
  chore(release): Publish
  Fix Starter Library URL and update copy (gatsbyjs#10368)
  feat(gatsby-source-filesystem): keep original name of remote files (gatsbyjs#9777)
  docs(gatsby-source-contentful): Rewrite of gatsby-source-contentful query section (gatsbyjs#7533)
  Update "Deploying with Now" Guide (gatsbyjs#10390)
  Add Matomo to list of analytics plugins (gatsbyjs#10372)
  Add satispay.com to showcase (gatsbyjs#10380)
  Adds @goblindegook/gatsby-starter-typescript (gatsbyjs#10377)
  Fix typo in gatsby-remark-code-repls sample `gatsby-config.json` in README (gatsbyjs#10361)
  fix(gatsby): fix false type conflict warning on plugin fields (gatsbyjs#10355)
  fix(docs): Just a small typo fix in the docs (gatsbyjs#10359)
  feat(gatsby-image): add onStartLoad prop  (gatsbyjs#6702)
  fix(docs): add Ecosystem to docs sidebar, consistency with tutorial sidebar (gatsbyjs#10350)
  fix(www): Starters.yaml housekeeping (gatsbyjs#10354)
  docs: add ttag starter (gatsbyjs#10352)
  docs: document branching (gatsbyjs#9983)
  plugin checker initial commit (gatsbyjs#7062)
  docs: new starter features is required (gatsbyjs#10353)
  docs: migrated line highlighting to highlight-line, highlight-start, highlight-end (gatsbyjs#10202)
  Add Birra Napoli to site showcase (gatsbyjs#10344)
  ...
m-allanson added a commit to alexkirsz/gatsby that referenced this pull request Dec 12, 2018
* master: (1425 commits)
  showcase: Add TMDb Gatsby site (gatsbyjs#10411)
  chore(www): bump offline plugin version (gatsbyjs#10409)
  chore(release): Publish
  Fix Starter Library URL and update copy (gatsbyjs#10368)
  feat(gatsby-source-filesystem): keep original name of remote files (gatsbyjs#9777)
  docs(gatsby-source-contentful): Rewrite of gatsby-source-contentful query section (gatsbyjs#7533)
  Update "Deploying with Now" Guide (gatsbyjs#10390)
  Add Matomo to list of analytics plugins (gatsbyjs#10372)
  Add satispay.com to showcase (gatsbyjs#10380)
  Adds @goblindegook/gatsby-starter-typescript (gatsbyjs#10377)
  Fix typo in gatsby-remark-code-repls sample `gatsby-config.json` in README (gatsbyjs#10361)
  fix(gatsby): fix false type conflict warning on plugin fields (gatsbyjs#10355)
  fix(docs): Just a small typo fix in the docs (gatsbyjs#10359)
  feat(gatsby-image): add onStartLoad prop  (gatsbyjs#6702)
  fix(docs): add Ecosystem to docs sidebar, consistency with tutorial sidebar (gatsbyjs#10350)
  fix(www): Starters.yaml housekeeping (gatsbyjs#10354)
  docs: add ttag starter (gatsbyjs#10352)
  docs: document branching (gatsbyjs#9983)
  plugin checker initial commit (gatsbyjs#7062)
  docs: new starter features is required (gatsbyjs#10353)
  ...
@jbolda
Copy link
Contributor

jbolda commented Dec 21, 2018

This has appeared to introduce a slight regression. In gatsby-source-airtable, the api delivers files with symbols/spaces/etc URI encoded and it appears that this update no longer handles it as it did previously. I don't have time right now to dig in further, but it appears that some functions expect it decoded and some don't?

This example reproduces the issue. There are multiple photos where this errors, but I know specifically the last one errors out. Unfortunately, to run it, you need an airtable account to get an API key, but you can just copy over the template. Essentially, the photo never loads because the url to the photo is incorrect.

@LekoArts
Copy link
Contributor

@jbolda #10650 might help?

@jbolda
Copy link
Contributor

jbolda commented Dec 25, 2018

@LekoArts I believe you are correct 👍

@oorestisime
Copy link
Contributor

FYI #11037

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.

6 participants