Add canonical link tags #6466

willdurand · 2018-10-01T13:15:00Z

This PR adds canonical link tags to the following pages:

⚠️ in order to avoid hardcoded URLs, I read the router config (pathname) from the redux state. It ensures that our canonical URLs are in sync with the router config, but that's why test cases don't have the "production" values. It also avoids issues with trailing slashes (because we can trust the middleware and we don't have to think about them).

codecov-io · 2018-10-01T13:25:32Z

Codecov Report

Merging #6466 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    mozilla/addons-frontend#6466      +/-   ##
==========================================
+ Coverage   97.82%   97.82%   +<.01%     
==========================================
  Files         239      239              
  Lines        6475     6488      +13     
  Branches     1238     1239       +1     
==========================================
+ Hits         6334     6347      +13     
  Misses        126      126              
  Partials       15       15

Impacted Files	Coverage Δ
src/amo/pages/Addon/index.js	`100% <ø> (ø)`	⬆️
src/amo/pages/Home/index.js	`100% <ø> (ø)`	⬆️
src/amo/store.js	`100% <ø> (ø)`	⬆️
src/amo/pages/Category/index.js	`100% <ø> (ø)`	⬆️
src/amo/pages/LandingPage/index.js	`100% <ø> (ø)`	⬆️
src/core/components/ServerHtml/index.js	`100% <ø> (ø)`	⬆️
src/amo/pages/StaticPages/ReviewGuide/index.js	`100% <100%> (ø)`	⬆️
src/amo/pages/LanguageTools/index.js	`100% <100%> (ø)`	⬆️
src/amo/utils.js	`100% <100%> (ø)`	⬆️
src/amo/pages/StaticPages/About/index.js	`100% <100%> (ø)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cda25ae...24d80d9. Read the comment docs.

rebmullin

@willdurand, I have some minor comments/questions noted but this looks really good to me 👍

r+wc

src/core/types/router.js

tests/unit/amo/helpers.js

src/amo/utils.js

rebmullin · 2018-10-05T14:58:36Z

config/lib/shared.js

@@ -6,4 +6,8 @@ export const apiDevHost = 'https://addons-dev.allizom.org';
 export const apiProdHost = 'https://addons.mozilla.org';
 export const apiStageHost = 'https://addons.allizom.org';

+export const baseUrlDev = 'https://addons-dev.allizom.org';
+export const baseUrlProd = 'https://addons.mozilla.org';
+export const baseUrlStage = 'https://addons.allizom.org';


since these are the same as above, could we use those?

you mean: baseUrlDev = apiDevHost?

yeh that's what I was thinking (since it's the same)

kumar303

I have some questions and change requests. I'm not sure that it's a good idea to execute the URL calculation in every mapStateToProps so I offered an alternative. Let me know what you think.

kumar303 · 2018-10-05T19:12:29Z

src/amo/pages/LanguageTools/index.js

@@ -67,14 +69,21 @@ export const LanguageToolList = ({ languageTools }: LanguageToolListProps) => {
 };

 type Props = {|
-  languageTools: Array<LanguageToolType>,
+  // eslint-disable-next-line react/no-unused-prop-types


We need this for pretty much any file that uses Flow so I'd just put it at the top.

src/core/types/router.js

tests/unit/amo/helpers.js

kumar303 · 2018-10-05T19:50:20Z

tests/unit/amo/test_utils.js

+      const lang = 'fr';
+
+      const _config = getFakeConfig({ baseURL });
+      const { state } = dispatchClientMetadata({ clientApp, lang });


Rather than rely on the implicit pathname set by dispatchClientMetadata, this should set an explicit pathname since that's integral to the test.

src/amo/pages/Addon/index.js

kumar303 · 2018-10-05T20:31:50Z

src/amo/pages/LandingPage/index.js

  const { landing, viewContext } = state;

  return {
    addonTypeOfResults: landing.addonType,
+    currentURL: getCurrentURL({ state, _config: ownProps._config }),


Rather than execute this in every mapStateToProps (which would happen a lot), how about just passing through the state we need? That would cause the function to only be executed when the state value changes and is more in line with how we typically do things in mapStateToProps.

Example:

diff --git a/src/amo/pages/LandingPage/index.js b/src/amo/pages/LandingPage/index.js index 67a639446..cceeb9a94 100644 --- a/src/amo/pages/LandingPage/index.js +++ b/src/amo/pages/LandingPage/index.js @@ -12,7 +12,7 @@ import { setViewContext } from 'amo/actions/viewContext'; import LandingAddonsCard from 'amo/components/LandingAddonsCard'; import NotFound from 'amo/components/ErrorPage/NotFound'; import Categories from 'amo/components/Categories'; -import { getCurrentURL } from 'amo/utils'; +import { getCanonicalURL } from 'amo/utils'; import { ADDON_TYPE_EXTENSION, ADDON_TYPE_THEME, @@ -212,11 +212,12 @@ export class LandingPageBase extends React.Component { render() { const { - currentURL, + _config, errorHandler, featuredAddons, highlyRatedAddons, loading, + locationPathname, trendingAddons, i18n, } = this.props; @@ -250,7 +251,10 @@ export class LandingPageBase extends React.Component { > <Helmet> <title>{headingText[addonType]}</title> - <link rel="canonical" href={currentURL} /> + <link + rel="canonical" + href={getCanonicalURL({ pathname: locationPathname, _config })} + /> </Helmet> {errorHandler.renderErrorIfPresent()} @@ -319,15 +323,15 @@ export class LandingPageBase extends React.Component { } export function mapStateToProps(state, ownProps) { - const { landing, viewContext } = state; + const { landing, router, viewContext } = state; return { addonTypeOfResults: landing.addonType, - currentURL: getCurrentURL({ state, _config: ownProps._config }), context: viewContext.context, featuredAddons: landing.featured.results, highlyRatedAddons: landing.highlyRated.results, loading: landing.loading, + locationPathname: router.location.pathname, trendingAddons: landing.trending.results, resultsLoaded: landing.resultsLoaded && landing.category === null, }; diff --git a/src/amo/utils.js b/src/amo/utils.js index f1f96a1b3..82f0db32e 100644 --- a/src/amo/utils.js +++ b/src/amo/utils.js @@ -9,6 +9,7 @@ import NotFound from 'amo/components/ErrorPage/NotFound'; import ServerError from 'amo/components/ErrorPage/ServerError'; import { makeQueryString } from 'core/api'; import type { AppState } from 'amo/store'; +import type { ReactRouterLocationType } from 'core/types/router'; export function getErrorComponent(status: number | null) { switch (status) { @@ -56,6 +57,16 @@ export const makeQueryStringWithUTM = ({ }); }; +export const getCanonicalURL = ({ + _config = config, + pathname, +}: {| + _config?: typeof config, + pathname: string, +|}): string => { + return `${_config.get('baseURL')}${pathname}`; +}; + export const getCurrentURL = ({ _config = config, state,

Rather than execute this in every mapStateToProps (which would happen a lot), how about just passing through the state we need? That would cause the function to only be executed when the state value changes and is more in line with how we typically do things in mapStateToProps.

I think that does make a lot of sense. I knew something was wrong with this patch but could not see what.. Thanks for your input!

willdurand · 2018-10-08T09:33:21Z

updated and rebased

willdurand · 2018-10-09T14:39:16Z

re-rebased

kumar303

r+wc

I am uneasy about copying/pasting onLocatonChange but at least your approach is simple 🤷‍♀️ I dunno, I say ship it but maybe we can revisit it later.

kumar303 · 2018-10-09T14:17:26Z

src/amo/store.js

 import type { CreateStoreParams, CreateReducerType } from 'core/types/store';

-export type AppState = {|
+type AppStateWithoutRouter = {|


Could we call this AMOAppState or InternalAppState? The distinction seems to be that these are all our explicitly mapped reducers whereas AppState also includes reducers added by third party libraries. The name isn't a big deal, though.

Yep, I go for InternalAppState in case there are other reducers we have to move out of this set.

kumar303 · 2018-10-09T14:21:54Z

src/core/types/router.js

  hash: string, // e.g. #some-anchor
  key: string,
  pathname: string, // e.g. /en-US/firefox/addon/tab-mix-plus/reviews/
+  search: string, // e.g. ?q=search-string
+  state?: Object,


Maybe just add a comment here that sometimes location contains state but sometimes it doesn't (in my debugging, I don't see it). This comment would help dissuade someone from relying on this property. I realize that state? was always defined here (before your patch) but I only just noticed it 😬

tests/unit/amo/helpers.js

willdurand force-pushed the canonical-links branch 2 times, most recently from f44f0d5 to fcae912 Compare October 4, 2018 12:12

willdurand changed the title ~~[WIP] Add canonical link tags~~ Add canonical link tags Oct 4, 2018

willdurand requested review from bobsilverberg, kumar303 and rebmullin October 4, 2018 13:00

rebmullin self-assigned this Oct 4, 2018

rebmullin approved these changes Oct 5, 2018

View reviewed changes

kumar303 assigned kumar303 and unassigned rebmullin Oct 5, 2018

kumar303 suggested changes Oct 5, 2018

View reviewed changes

willdurand force-pushed the canonical-links branch from cd0f774 to 6118e2c Compare October 8, 2018 08:57

willdurand requested review from kumar303 and removed request for bobsilverberg October 8, 2018 09:23

willdurand added 13 commits October 9, 2018 16:37

add canonical link tag on addon detail pages

cb71512

Add baseURL config param

47c014c

Add more canonical links by reading the router state

bbf3d56

prettier

db2eb1e

Add canonical links on static pages

477bf31

fix lint errors

a8a1f29

fix test case desc

d7a23ae

add missing test case

efb2fba

Switch to getCanonicalURL impl

fcdff87

DRY

c2942a0

update tests

c2d3043

remove useless import

a6408ac

Expose link in server responses

36f13e0

willdurand force-pushed the canonical-links branch from 3d76ebe to 36f13e0 Compare October 9, 2018 14:39

kumar303 approved these changes Oct 9, 2018

View reviewed changes

fixes

24d80d9

willdurand mentioned this pull request May 5, 2024

Add a test case for onLocationChanged helper function/action mozilla/addons#12502

Closed

willdurand merged commit f863c91 into master Oct 9, 2018

willdurand deleted the canonical-links branch October 9, 2018 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add canonical link tags #6466

Add canonical link tags #6466

willdurand commented Oct 1, 2018 •

edited

codecov-io commented Oct 1, 2018 •

edited

rebmullin left a comment

rebmullin Oct 5, 2018

willdurand Oct 5, 2018

rebmullin Oct 5, 2018

kumar303 left a comment

kumar303 Oct 5, 2018

kumar303 Oct 5, 2018

kumar303 Oct 5, 2018

willdurand Oct 8, 2018

willdurand commented Oct 8, 2018

willdurand commented Oct 9, 2018

kumar303 left a comment

kumar303 Oct 9, 2018

willdurand Oct 9, 2018

kumar303 Oct 9, 2018

Add canonical link tags #6466

Add canonical link tags #6466

Conversation

willdurand commented Oct 1, 2018 • edited

codecov-io commented Oct 1, 2018 • edited

Codecov Report

rebmullin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willdurand commented Oct 8, 2018

willdurand commented Oct 9, 2018

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willdurand commented Oct 1, 2018 •

edited

codecov-io commented Oct 1, 2018 •

edited