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(gatsby-remark-images): ensure query string is ignored when detecting images #11743

Merged
merged 6 commits into from
Feb 15, 2019

Conversation

anotherjames
Copy link
Contributor

Description

Referencing an image in markdown like this:

![Alt text](../path/to/image.png?raw=true)

Does not load the image.

This PR uses query-string to parse the URL out of the path.

Related Issues

(No issue opened yet.)

This is my first PR, I hope it's ok!

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.

Looking good! Left a comment on how we can clean this up. Additionally, it'd be super nice if we could add a test or two to validate this functionality!

@@ -113,7 +116,7 @@ module.exports = (
const presentationWidth = fluidResult.presentationWidth

// Generate default alt tag
const srcSplit = node.url.split(`/`)
const srcSplit = queryString.parseUrl(node.url).url.split(`/`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not specific to your PR (this behavior already existed!), but this seems like a good opportunity for a helper, perhaps something like?

const getImageInfo = uri => {
  const { url, query } = queryString.parseUrl(uri)
  return {
    ext: path.extname(url),
    url,
    query
  }
}

Here's an example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a helper in 185f8ca. The filename without the extension is pulled out for a default alt tag, which could be added to the helper. But that's the only place that's done, so I thought best to leave that part unless it's needed elsewhere. I'm happy to move it into the helper too if wanted though :-)

I'll look into adding a test next.

@anotherjames
Copy link
Contributor Author

I don't understand why those tests have failed, the failures don't look relevant to me? Anyone that does, please help, thanks! :-)

@DSchau
Copy link
Contributor

DSchau commented Feb 14, 2019

@anotherjames we had a little hiccup with one of our dependencies this morning--all fixed now! I just merged upstream/master into your branch and we should be good to go as far as tests passing now!

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.

Left a comment/request, thanks!

@@ -328,7 +338,7 @@ module.exports = (
return resolve()
}

const fileType = formattedImgTag.url.slice(-3)
const fileType = getImageInfo(formattedImgTag.url).ext
Copy link
Contributor

Choose a reason for hiding this comment

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

path.extname returns a file extension with a dot. In other words, given const file = 'asdfasdf.png'

  • path.extname(file) === '.png'
  • file.slice(-3) === 'png'

Wouldn't this then break the subsequent checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yes, sorry!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should now be resolved by d10a826. Thanks so much for you guidance throughout this PR!

@DSchau DSchau added the status: awaiting author response Additional information has been requested from the author label Feb 14, 2019
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.

Looks great! Validated with a sample repo, and doesn't seem to have caused any regressions and does fix the issue you describe.

Let's merge!

@DSchau DSchau changed the title Parse path out of image URLs so that query strings are ignored. fix(gatsby-transformer-remark): ensure query string is ignored when detecting images Feb 15, 2019
@DSchau DSchau changed the title fix(gatsby-transformer-remark): ensure query string is ignored when detecting images fix(gatsby-remark-images): ensure query string is ignored when detecting images Feb 15, 2019
@DSchau DSchau merged commit 37cc21f into gatsbyjs:master Feb 15, 2019
@gatsbot
Copy link

gatsbot bot commented Feb 15, 2019

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

@DSchau
Copy link
Contributor

DSchau commented Feb 15, 2019

Published as gatsby-remark-images@3.0.4

Thanks for the contribution!

@anotherjames
Copy link
Contributor Author

Thank you for the wonderfully warm welcome! 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting author response Additional information has been requested from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants