-
Notifications
You must be signed in to change notification settings - Fork 3
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
fix: 156 obsolete browsers #159
Conversation
a75b3b4
to
a6e95ad
Compare
Codecov Report
@@ Coverage Diff @@
## main #159 +/- ##
==========================================
+ Coverage 71.84% 71.85% +0.01%
==========================================
Files 126 126
Lines 10108 10106 -2
==========================================
Hits 7262 7262
+ Misses 2846 2844 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
<script> | ||
/** Reflect ANY changes to this script in the script hash in `server/eb/app.py` **/ | ||
var root = document.getElementById("root"); | ||
root.remove(); | ||
var portals = document.getElementsByClassName("bp3-portal"); | ||
for (var i = 0; i < portals.length; i += 1) { | ||
portals[i].remove(); | ||
} | ||
</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.
We're going to need to update CSP
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.
Ah thank you! Yeah we can remove the associated hash now! I'll add it as a TODO in the PR and ticket descriptions 👍
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.
OH! It's just in the BE code not in Infra repo lol Updated! Thank you!
a6e95ad
to
078e2de
Compare
@seve PTAL again thank you 🙏 |
<script | ||
async | ||
src="static/obsoleteBrowsers.js" | ||
id="obsolete-browsers" | ||
data-template="<%= OBSOLETE_TEMPLATE %>" | ||
data-regex="<%= OBSOLETE_REGEX %>" | ||
></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.
Is this going to need to be reflected in CSP?
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.
Great question! Since the script will be served from the same domain as the HTML page, script-src "'self'"
should cover it. Only inline scripts need hashes 👍
@@ -0,0 +1,22 @@ | |||
/* eslint-disable @blueprintjs/classes-constants -- do not import files */ |
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.
Maybe this too will need to be included in CSP?
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.
Please see comment above!
Reviewers
Functional:
@seve
Readability:
@dimple-jethani
Changes
This PR replaces
obsolete-webpack-plugin
and usesbrowserslist-useragent-regexp
to generate a REGEX to test thenavigator.userAgent
insteadThe strategy is:
browserslist-useragent-regexp
to generate REGEX based onbrowserslist
and output a fileconfiguration/webpack/SUPPORTED_BROWSERS_REGEX.js
configuration/webpack/webpack.config.shared.js
imports the REGEX and obsolete html template and inject them into bothindex.html
andindex_template.html
on the designated script tag.configuration/webpack/webpack.config.shared.js
also copies the static fileconfiguration/webpack/obsoleteBrowsers.js
into the/build/static/
directory, so the servers know how to handle the request for the fileindex.html
andindex_template.html
then loads the script file script tag in step 2 viasrc="obsoleteBrowsers.js"
. NOTE: we continue to use relative path here instead of absolute path, so the request will still be prefixed with data root path (the namespace needed for CloudFront)obsoleteBrowsers.js
is now downloaded asynchronously and parse REGEX and HTML template from its script tag attributes and test the browser and render the obsolete HTML as neededSafari:
Chrome:
🚨🚨🚨 TODO 🚨🚨🚨
1. Still need to remove associated script hash inFalse alarmsingle-cell-infra
repo