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-plugin-canonical-urls): add option to strip query string #13339

Merged
merged 8 commits into from
May 16, 2019

Conversation

Ehesp
Copy link
Contributor

@Ehesp Ehesp commented Apr 13, 2019

[gatsby-plugin-canonical-urls]

cc @sidharthachatterjee @MarkSalabutin

Description

This is a fairly opinionated PR. I can flip the logic if needed 👍

Issue #12709 makes a good point about the search params (e.g. ?tag=foobar) being added to the canonical urls this plugin generates. From my limited SEO knowledge, the search params being included makes this plugin somewhat redundant.

A canonical tag (aka "rel canonical") is a way of telling search engines that a specific URL represents the master copy of a page. Using the canonical tag prevents problems caused by identical or "duplicate" content appearing on multiple URLs. Practically speaking, the canonical tag tells search engines which version of a URL you want to appear in search results. https://moz.com/learn/seo/canonicalization

Lets say I have the following page: /blog

If I add a tagging system, and have functionality for /blog?tag=foobar, search engines will still index this as a separate page, even with this plugin installed. The master reference is in-fact /blog. This may not be the case all of the time, but I can't think of many cases, especially in Gatsby, where you'd want a "duplicate page" to be indexed when search params are different - it'd make more sense to create a totally different page/route.

These changes means search params won't be added by default, but gives the user the ability to set a flag adding them on. As mentioned above, I can flip this logic if you guys think it's too opinionated to do that 😄

It would be good for someone else to test this, as breaking this functionality can have pretty serious consequences for websites SEO!

Related Issues

#12709

@pieh
Copy link
Contributor

pieh commented Apr 14, 2019

Please don't change version and changelog manually - this happen automatically when we publish new release.

Given breaking nature of proposed change (in current shape) - I would vote for introducing option to strip search / query from canonical link (so it's not breaking for anyone who relies on current behavior) and do some research for any cases when having query string is desirable and figure out what's best default behavior - if we will end up deciding that striping search by default is best we would just flip default value for strip option and bump major version then.

@Ehesp
Copy link
Contributor Author

Ehesp commented Apr 14, 2019

@pieh no problem will revert the changelog/package.json changes.

That makes sense, I'll revert the logic so the params can be excluded.

@Ehesp
Copy link
Contributor Author

Ehesp commented Apr 17, 2019

Updated to exclude search params if search is specifically set to false.

freiksenet
freiksenet previously approved these changes May 16, 2019
@pieh pieh changed the title [gatsby-plugin-canonical-urls] Only append search parameters if required feat(gatsby-plugin-canonical-urls] Only append search parameters if required May 16, 2019
@pieh pieh changed the title feat(gatsby-plugin-canonical-urls] Only append search parameters if required feat(gatsby-plugin-canonical-urls): add option to strip query string May 16, 2019
@pieh
Copy link
Contributor

pieh commented May 16, 2019

What do you think about renaming stripSearchParam to stripQueryString as query string is actual name of that part of urls (so technically more correct :P)?

Other than that, it LGTM

@LekoArts
Copy link
Contributor

@pieh changed the name :D

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.

LGTM! Thanks @Ehesp and @LekoArts!

@pieh pieh merged commit c903fed into gatsbyjs:master May 16, 2019
@Ehesp Ehesp deleted the fix-12709 branch May 20, 2019 15:20
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.

4 participants