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

Add redirector worker #1

Merged
merged 2 commits into from Oct 10, 2019

Conversation

@alexgibson
Copy link
Member

alexgibson commented Oct 3, 2019

@alexgibson

This comment has been minimized.

Copy link
Member Author

alexgibson commented Oct 3, 2019

@jgmize r?

@alexgibson alexgibson requested a review from jgmize Oct 3, 2019
@alexgibson alexgibson referenced this pull request Oct 3, 2019
3 of 5 tasks complete
workers/redirector.js Outdated Show resolved Hide resolved
test/redirector-test.js Outdated Show resolved Hide resolved
workers/redirector.js Outdated Show resolved Hide resolved
@pmac

This comment has been minimized.

Copy link
Member

pmac commented Oct 3, 2019

It seems possible to return the bedrock response directly instead of redirecting. Would that be desirable, or do we for analytics and/or SEO reasons not want to do that?

@alexgibson

This comment has been minimized.

Copy link
Member Author

alexgibson commented Oct 3, 2019

It seems possible to return the bedrock response directly instead of redirecting. Would that be desirable, or do we for analytics and/or SEO reasons not want to do that?

I think the answer here is that shadowing could confuse Google since the content would keep changing, but let me double check with Raphael.

@alexgibson

This comment has been minimized.

Copy link
Member Author

alexgibson commented Oct 3, 2019

Ok, I double checked with Raphael and he says as long as our experimental pages have a canonical that points to the original URLs (which they do), then a 302 is fine. Shadowing is also ok, but only as long as experiments aren't drastically changing the page. Shadowing is a little more risky, since copy changes could essentially get indexed.

"url": "https://github.com/mozmeao/www-workers/issues"
},
"devDependencies": {
"@dollarshaveclub/cloudworker": "^0.1.1",

This comment has been minimized.

Copy link
@metadave

metadave Oct 3, 2019

Member

wow, never thought I'd see dollarshaveclub in a PR :-)

@alexgibson

This comment has been minimized.

Copy link
Member Author

alexgibson commented Oct 4, 2019

@jgmize I pushed a commit with some review fixes, plus a couple of other small details. Here's a summary:

  • Removed origin as a variable (switching to relative paths in the config).
  • Set sample rate for /en-US/firefox/new/ to 6%.
  • Added an additional unit test that checks query params are being preserved on redirect URLs.
  • Unit tests now use https://bedrock-stage.gcp.moz.works/ for an origin, so they won't conflict with a real worker running on www-dev etc.
  • Added ESLint config check to CI.
  • Added .editorconfig file.

I've set the updated worker script running on https://www.allizom.org/en-US/firefox/new/ and done some manual testing. It still seems to be working as expecte 👍

@alexgibson alexgibson force-pushed the add-redirector-worker branch from d84e588 to 14d6592 Oct 4, 2019
@jgmize
jgmize approved these changes Oct 10, 2019
Copy link
Member

jgmize left a comment

Nice work @alexgibson!

@jgmize jgmize merged commit 4504688 into master Oct 10, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jgmize jgmize deleted the add-redirector-worker branch Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.