-
Notifications
You must be signed in to change notification settings - Fork 258
WS-1400: Add client side redirect #13407
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
base: latest
Are you sure you want to change the base?
Changes from all commits
493f7bd
717d0bb
01d4e4d
c994aba
2bea896
70e7ad7
6359b3a
15f88f5
d5c84a4
9bac75e
1e086de
4cdf217
f01ad51
a606199
4dfd6cc
6832436
dc3594d
aa80aaf
2a2be1c
e0dbde9
5f2df6e
97dfa11
48451c3
1571678
83a1a3b
657b4c2
501c8cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| import { redirectScript } from '.'; | ||
|
|
||
| describe('LiteRedirect', () => { | ||
| it.each([ | ||
| { | ||
| effectiveType: 'randomValue', | ||
| expectedRedirect: false, | ||
| }, | ||
| { | ||
| effectiveType: 'slow-2g', | ||
| expectedRedirect: true, | ||
| }, | ||
| { | ||
| effectiveType: '2g', | ||
| expectedRedirect: true, | ||
| }, | ||
| { | ||
| effectiveType: '3g', | ||
| expectedRedirect: true, | ||
| }, | ||
| { | ||
| effectiveType: '4g', | ||
| expectedRedirect: false, | ||
| }, | ||
| ])( | ||
| `When the client is on $effectiveType then expect redirect should be $expectRedirect`, | ||
| ({ effectiveType, expectedRedirect }) => { | ||
| const mockWindow = { | ||
| navigator: { | ||
| connection: { | ||
| effectiveType, | ||
| }, | ||
| }, | ||
| location: { | ||
| replace: jest.fn(), | ||
| href: 'https://www.somepath.com/', | ||
| pathname: '/pidgin/articles/czrzwn80zjmo', | ||
| }, | ||
| } as unknown as Window; | ||
|
|
||
| redirectScript(mockWindow); | ||
| const replaceCallStack = (mockWindow.location.replace as jest.Mock).mock | ||
| .calls[0]?.[0]; | ||
|
|
||
| const hasRedirected = Boolean(replaceCallStack); | ||
|
|
||
| expect(hasRedirected).toBe(expectedRedirect); | ||
| }, | ||
| ); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| import React from 'react'; | ||
|
|
||
| export const redirectScript = (window: Window) => { | ||
| const { pathname } = window.location; | ||
|
|
||
| const allowList = ['/pidgin/articles/czrzwn80zjmo']; | ||
| if ( | ||
| window?.navigator?.connection?.effectiveType && | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Older browsers like Opera Mini don’t support optional chaining I don't think, so they might hit a
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @amoore108 any thoughts on this? ^
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The script should be transpiled down by the time its rendered in the browser. Worth checking locally that that is the case. |
||
| allowList.includes(pathname) | ||
| ) { | ||
| const toLitePath = `${pathname}.lite`; | ||
| const ect = window.navigator.connection.effectiveType; | ||
| const normalisedEct = ect.toLocaleLowerCase(); | ||
| switch (normalisedEct) { | ||
| case 'slow-2g': | ||
| case '2g': | ||
| case '3g': | ||
| window.location.replace(toLitePath); | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| // THIS COMPONENT IS ONLY TO BE USED WITH CANONICAL RENDERERS | ||
| // DO NOT USE IT WITH LITE AND AMP RENDERERS | ||
| export default () => { | ||
| return ( | ||
| <script> | ||
| {` | ||
| window.addEventListener('load', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't we use https://developer.mozilla.org/en-US/docs/Web/API/Document/DOMContentLoaded_event instead? The window load will happen after all HTML/JSON/CSS/images load so perhaps too long? |
||
| (${redirectScript.toString()})(window) | ||
| }) | ||
| `} | ||
| </script> | ||
| ); | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not convinced we'll get 5g through as per the docs - https://developer.mozilla.org/en-US/docs/Glossary/Effective_connection_type (though I know that was there before)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, I put this one here because it partially clashed with the hook that Neon are working on. I don't want to make any changes in case it affects their work.