feat: add support for query params in canonical url #1274

RobbieTheWagner · 2021-08-31T13:31:42Z

This is a first attempt at adding support for including certain query params in the canonical url. Currently, it uses global options, but I am open to making it work on a page by page basis, I just wasn't sure how to hook that up.

Related to #912

divine · 2021-08-31T14:23:28Z

Hello,

We're already using ufo, so I'd suggest instead of manually adjusting the URL, use ufo's function https://github.com/unjs/ufo#getquery

Trailing slashes should be covered?

Thanks!

rchl · 2021-08-31T18:38:47Z

We're already using ufo, so I'd suggest instead of manually adjusting the URL, use ufo's function unjs/ufo#getquery

Or even use URL but simplify a lot since URL object already contains parsed params (searchParams) that can be modified and then the URL can be stringified back.

But before focusing on details, we have to look at the big picture. My initial reaction was that we shouldn't introduce global option like that because we'll end up with a mess later, if we need to also introduce it per-page. And we can't just remove it later since that would be a breaking change. Though maybe global option like that would also be useful to support but then we should think of the best way to do that all together.

One idea I can think of is passing those options as arguments to $nuxtI18nHead. Seems like the most natural place to me. Unless there are some issues with that that I can't think of. That said, it would still have to support defining those options per-route since $nuxtI18nHead can be used from global places like nuxt.config.js or a layout file.

divine · 2021-08-31T18:56:50Z

Well, $nuxtI18nHead would be the best choice to add that option and it definitely makes sense!

Thanks!

RobbieTheWagner · 2021-08-31T19:37:27Z

@rchl @divine I defer to whatever you all think is best, I just wanted to get this started as a proof of concept.

divine · 2021-09-22T12:26:42Z

Hello @rwwagner90,

Have you successfully implemented this feature? If not, I could try to take care.

Thanks!

RobbieTheWagner · 2021-09-22T13:11:43Z

@divine I have not had the time to dedicate to this. If you have time, and would like to implement it, please feel free, I would love the help! 😄

RobbieTheWagner · 2021-09-27T21:32:53Z

@divine I updated the PR. Can you please approve the workflow so the tests can run?

divine · 2021-09-27T22:06:19Z

@divine I updated the PR. Can you please approve the workflow so the tests can run?

Hello,

I don't have permission to do that as I'm a just a contributor like you :).

I have done the similar work as you but didn't have a chance to complete it as well!

Can you also make canonical attibutes optional (true by default?).

If you want I can publish my branch to take a look.

Thanks!

RobbieTheWagner · 2021-09-27T22:22:35Z

@divine it is optional and defaults to an empty array.

RobbieTheWagner · 2021-09-27T23:39:08Z

@rchl can you approve the workflow please so we can see what tests do?

rchl · 2021-09-28T08:26:31Z

@rchl can you approve the workflow please so we can see what tests do?

Done.
BTW. You can run tests locally. For example this should run only your new tests:

yarn test:e2e-ssr -t 'canonical SEO link'

divine · 2021-09-28T08:45:01Z

@divine it is optional and defaults to an empty array.

Hello,

What I mean is to add an extra parameter to make canonical optional (default true):

export function nuxtI18nHead ({ addCanonicalAttributes = true ...

Later:

function addCanonicalLinks (baseUrl, link) {
  if (!addCanonicalAttributes) {
    return
  }

And this logic might be better:

let href = toAbsoluteUrl(canonicalPath, baseUrl)

if (currentRoute && addCanonicalQueries.length) {
  const params = new URLSearchParams()
  for (const name in addCanonicalQueries) {
    const value = currentRoute.query[name]
    if (Array.isArray(value)) {
      value.forEach(v => params.append(name, v || ''))
    } else {
      params.append(name, value || '')
    }
  }

  href = `${href}?${params.toString()}`
}

What do you think @rchl?

Thank!

RobbieTheWagner · 2021-09-28T16:29:58Z

@divine making canonical optional entirely seems out of scope for this PR. This PR is to add the ability to specify which query params should be included in the canonical link.

RobbieTheWagner · 2021-09-28T17:28:51Z

@rchl looks like it needs to be approved again, but I ran it locally and it seemed to be working. Please let me know if I need to make any further changes!

RobbieTheWagner · 2021-09-28T18:27:45Z

I keep getting implicitly has an 'any[]' type errors for canonicalQueries, even though I correctly added the types. To appease the linter I will make it false by default, instead of an empty array, but it would be great to not have to make it both a boolean and an array.

RobbieTheWagner · 2021-09-28T18:28:58Z

@rchl I have fixed the lint issues now, if you want to approve again.

rchl

The paragraph at the bottom of the https://i18n.nuxtjs.org/seo documentation page should also be updated as it got outdated with those changes

docs/content/en/api.md

src/templates/head-meta.js

test/module.test.js

types/vue.d.ts

Co-authored-by: Rafał Chłodnicki <rchl2k@gmail.com>

divine · 2021-09-29T03:23:04Z

@divine making canonical optional entirely seems out of scope for this PR. This PR is to add the ability to specify which query params should be included in the canonical link.

Why not? It won't take you a minute, anyways, I'll do on my own then.

Thanks!

RobbieTheWagner · 2021-09-29T20:51:56Z

Why not? It won't take you a minute, anyways, I'll do on my own then.

@divine just because it is not related to this PR at all. This is to add support for adding query params to canonical urls. Once it is in, I would be glad to help with any additional options you think need to be added.

Co-authored-by: Rafał Chłodnicki <rchl2k@gmail.com>

RobbieTheWagner · 2021-09-29T21:42:01Z

@rchl I tried to support setting ?foo with no value, but URLSearchParams does not support that, so I added some hacks to remove the null and undefined ones later:

   queryString = queryString.replace('=undefined', '')
   queryString = queryString.replace('=null', '')

Let me know if you have any better ideas. I think checking that the value exists and is a string is maybe good, to avoid these hacks. I would assume people don't use query params with no value often?

src/templates/head-meta.js

RobbieTheWagner · 2021-10-01T01:36:09Z

@rchl is there anything else I need to update here to get this PR in?

rchl · 2021-10-06T19:37:44Z

The part where I asked to change the types of canoncialQueries is not addressed.

rchl · 2021-10-06T19:55:32Z

Also this is not addressed: #1274 (review)

rchl · 2021-10-06T20:00:12Z

I'm also thinking about folding the new canonicalQueries option into the addSeoAttributes one. Would do that by accepting an object for addSeoAttributes with more detailed configuration.

It feels a bit wrong now because canonicalQueries is a separate option but it really depends on addSeoAttributes being true.

I can do that change myself if you are feeling like I'm pushing too much. :)

src/templates/head-meta.js

RobbieTheWagner · 2021-10-06T22:27:46Z

The part where I asked to change the types of canoncialQueries is not addressed.

@rchl I see you pushed a change to fix the types, thanks! I could not figure it out 😬

Also this is not addressed: #1274 (review)

Could you elaborate on what is needed here please?

rchl · 2021-10-07T06:25:04Z

Could you elaborate on what is needed here please?

It says:

When the page contains the query parameters, the canonical link will not include query params. ... Note that there is currently no way to override that in case that including a specific query param would be desired.

So it's obviously outdated now. Needs to be changed to mention how to use the new functionality to enable that.

RobbieTheWagner · 2021-10-08T14:01:34Z

@rchl I updated those docs. We still need to address this issue #1274 (comment)

src/templates/head-meta.js

divine · 2021-10-08T14:28:38Z

Query strings which behavior is kinda weird and my suggestion would be to use something like https://github.com/sindresorhus/query-string which covers all of that problems.

That means depending on another library as well... but at least it will make things stable.

Forgot to add, that this should be configurable skipEmptyString skipNull.

RobbieTheWagner · 2021-10-08T16:30:04Z

Query strings which behavior is kinda weird and my suggestion would be to use something like https://github.com/sindresorhus/query-string which covers all of that problems.

That means depending on another library as well... but at least it will make things stable.

Forgot to add, that this should be configurable skipEmptyString skipNull.

I feel we should just use the code as I wrote it, which strips null and undefined, but I don't feel super strongly. Technically, query params are always undefined if you don't define them, so I consider that an invalid edge case.

RobbieTheWagner · 2021-10-08T16:31:39Z

@rchl @divine you seem to have strong opinions about including || '' and changing the way I had things to support ?foo. I really don't care too much, and just want to get this feature in. Please let me know what needs to be done to get this merged. I will never use an invalid query param with no value like ?foo anyway, so this PR is ready to go for our use cases 😄

rchl · 2021-10-17T18:26:08Z

Sorry for the delay.
I will now also attempt to merge the new option into addSeoAttributes and push the changes here.

rchl · 2021-10-17T18:58:52Z

@rwwagner90 please have a look and let me know if you are fine with my changes.

RobbieTheWagner · 2021-10-17T23:08:41Z

@rchl looks good to me! 🚢

RobbieTheWagner · 2021-10-18T15:18:14Z

Thanks for merging @rchl! When do you think a new release will be out?

rchl · 2021-10-19T09:33:20Z

Released https://github.com/nuxt-community/i18n-module/releases/tag/v7.1.0

Add support for query params in canonical ref

df6f510

RobbieTheWagner mentioned this pull request Aug 31, 2021

Canonical URL for prefixed strategy #912

Closed

Update implementation

968e397

RobbieTheWagner changed the title ~~Add support for query params in canonical url~~ feat: add support for query params in canonical url Sep 27, 2021

Don't use URL

3983ff7

RobbieTheWagner added 2 commits September 28, 2021 13:10

Update logic

3cf05c5

Update configs

4eae7a2

Fix lint

3cba586

rchl requested changes Sep 28, 2021

View reviewed changes

docs/content/en/api.md Outdated Show resolved Hide resolved

src/templates/head-meta.js Outdated Show resolved Hide resolved

src/templates/head-meta.js Outdated Show resolved Hide resolved

test/module.test.js Outdated Show resolved Hide resolved

types/vue.d.ts Outdated Show resolved Hide resolved

RobbieTheWagner and others added 3 commits September 28, 2021 21:17

Update docs/content/en/api.md

6eeabf2

Co-authored-by: Rafał Chłodnicki <rchl2k@gmail.com>

Update types/vue.d.ts

47dc7ab

Co-authored-by: Rafał Chłodnicki <rchl2k@gmail.com>

Add page level test

cdf0494

RobbieTheWagner and others added 2 commits September 29, 2021 17:11

Update src/templates/head-meta.js

c1a7541

Co-authored-by: Rafał Chłodnicki <rchl2k@gmail.com>

Forcibly remove null and undefined

615f81f

rchl reviewed Sep 30, 2021

View reviewed changes

src/templates/head-meta.js Outdated Show resolved Hide resolved

better types for canonicalQueries

bd2f588

RobbieTheWagner commented Oct 6, 2021

View reviewed changes

src/templates/head-meta.js Show resolved Hide resolved

Update seo.md

26dcf53

divine reviewed Oct 8, 2021

View reviewed changes

src/templates/head-meta.js Show resolved Hide resolved

Merge options into addSeoAttributes

2b0e5a8

revert pointless change

371e399

rchl merged commit d5dea9c into nuxt-modules:main Oct 18, 2021

RobbieTheWagner deleted the canonical-queries branch October 18, 2021 15:17

feat: add support for query params in canonical url #1274

feat: add support for query params in canonical url #1274

Conversation

RobbieTheWagner commented Aug 31, 2021

divine commented Aug 31, 2021

rchl commented Aug 31, 2021

divine commented Aug 31, 2021

RobbieTheWagner commented Aug 31, 2021

divine commented Sep 22, 2021

RobbieTheWagner commented Sep 22, 2021

RobbieTheWagner commented Sep 27, 2021

divine commented Sep 27, 2021 • edited Loading

RobbieTheWagner commented Sep 27, 2021

RobbieTheWagner commented Sep 27, 2021

rchl commented Sep 28, 2021 • edited Loading

divine commented Sep 28, 2021

RobbieTheWagner commented Sep 28, 2021

RobbieTheWagner commented Sep 28, 2021

RobbieTheWagner commented Sep 28, 2021

RobbieTheWagner commented Sep 28, 2021

rchl left a comment • edited Loading

Choose a reason for hiding this comment

divine commented Sep 29, 2021

RobbieTheWagner commented Sep 29, 2021

RobbieTheWagner commented Sep 29, 2021

RobbieTheWagner commented Oct 1, 2021

rchl commented Oct 6, 2021

rchl commented Oct 6, 2021

rchl commented Oct 6, 2021

RobbieTheWagner commented Oct 6, 2021

rchl commented Oct 7, 2021

RobbieTheWagner commented Oct 8, 2021

divine commented Oct 8, 2021 • edited Loading

RobbieTheWagner commented Oct 8, 2021

RobbieTheWagner commented Oct 8, 2021

rchl commented Oct 17, 2021

rchl commented Oct 17, 2021

RobbieTheWagner commented Oct 17, 2021

RobbieTheWagner commented Oct 18, 2021

rchl commented Oct 19, 2021

divine commented Sep 27, 2021 •

edited

Loading

rchl commented Sep 28, 2021 •

edited

Loading

rchl left a comment •

edited

Loading

divine commented Oct 8, 2021 •

edited

Loading