Browser support config by ranbena · Pull Request #3609 · getredash/redash

ranbena · 2019-03-19T22:34:18Z

What type of PR is this?

Feature

Description

Using browserslist to define which browser versions Redash supports.
List established from minimum "Async function" browser feature support (caniuse).

// package.json
"browserslist": [
  "Edge >= 15",
  "Firefox >= 52",
  "Chrome >= 55",
  "Safari >= 10.1",
  "iOS >= 10.3",
  "Opera >= 42",
  "op_mob >= 46",
  "android >= 67",
  "and_chr >= 71",
  "and_ff >= 64",
  "and_uc >= 11.8",
  "samsung >= 6.2"
]

This list allows us to utilize some powerful tools (all adhering to list automatically):

Babel transpiler 🚯
- js reduced by 2k (comparing app.js on netlify preview)
- css no change (autoprefix still must use -ms for Edge browser)
- although babel should drop "async func" and "arrow func" polyfills according to this list - it doesn't. So I excluded these explicitly in .babelrc.
Eslint rule - error on unsupported browser feature usage 👮
- Also available with stylelint but doesn't support less file so whatever man.
Outdated browser detection ~~with browserslist-useragent~~ only IE for now🚦
- redirecting to a dedicated "browser not supported" static page (TBD).

ranbena · 2019-03-19T22:50:58Z


+if (!matchesUA(navigator.userAgent, { allowHigherVersions: true })) {
+  // TODO: create "browser outdated" page and redirect to it
+  console.log('Your browser is outdated', resolveUserAgent(navigator.userAgent));


Is this the place (and method) to put the redirect?

Yes, it's the right place. And probably you don't need a page for redirect: create a component with message or whatever, and conditionally show it instead of <div ng-view>

But then we're still running potentially unsupported js for that page, which might fail it.
If we early redirect to a static html page, we don't have to worry about it.

If you detect UA on client-side - you always have a chance to run unsupported code. If you'll change app-view/template.html to something like this - no change it will render header/footer/page:

<div ng-if="$ctrl.isBrowserSupported">  </div> <div ng-if="!$ctrl.isBrowserSupported"> Error! Error!!!! Unsupported browser!!!!111 </div>

It's important that app-view component itself shouldn't use any unsupported features, as well as all code it uses via imports.

Sounds easy and simple.
I'll do that and also add a cypress test to make sure it doesn't break.

@kravets-levko does this make sense? (I'd rather it have it's own template)

export default function init(ngModule) { ngModule.factory( '$exceptionHandler', () => function exceptionHandler(exception) { handler.process(exception); }, ); const isBrowserSupported = someCheck(...); ngModule.component('appView', { template: isBrowserSupported ? template : notSupportedTemplate, controller: isBrowserSupported ? AppViewComponent : NotSupportedCtrl, }); }

@ranbena Yes, it's probably even better 👍

arikfr · 2019-03-20T09:00:39Z

I think the initial approach of redirect is the correct one. We should assume that an unsupported feature can break the whole bundle and cause the app not to load in which case we won’t show any message to the user. To avoid possible issues we should do the detection in a separate file we will include in index.html. Actually we can avoid redirect and just render a message. But point is: it should not depend on the bigger code base or on any of it working.

…

-- Arik Fraimovich

On Wed, Mar 20 2019 at 10:56 AM, < ***@***.*** > wrote: ***@***.**** commented on this pull request. In client/app/components/app-view/index.js ( #3609 (comment) ) : > import PromiseRejectionError from '@/lib/promise-rejection-error'; import { ErrorHandler } from './error-handler'; import template from './template.html'; +if (!matchesUA(navigator.userAgent, { allowHigherVersions: true })) { + // TODO: create "browser outdated" page and redirect to it + console.log('Your browser is outdated', resolveUserAgent(navigator.userAgent)); @ranbena ( https://github.com/ranbena ) Yes, it's probably even better 👍 — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub ( #3609 (comment) ) , or mute the thread ( https://github.com/notifications/unsubscribe-auth/AAEXLANRBiAZUjJumhgKj-8WTgnDVwv-ks5vYfeggaJpZM4b9QQl ).

ranbena · 2019-03-20T11:38:24Z

I think the initial approach of redirect is the correct one.

Best practice would be to determine and redirect in static.py.

@routes.route(org_scoped_rule('/<path:path>'))
@routes.route(org_scoped_rule('/'))
@supported_browser_required  <--
@login_required
def index(**kwargs):
    return render_index()

but that would require re-implementing browserlist-useragent in Python (currently available only in js and ruby).

But going with JS has a clear problem - it's not a standalone file, but a js module.

Options:

Back down and import it into app-view.js.
Implement it in python (and perhaps contribute to the open source project)
Reimplement as standalone js file to be loaded in index.html.

None are perfect, my personal pref is #2 (although not my comfort zone).
Wdyt @arikfr?

arikfr · 2019-03-20T13:06:03Z

Best practice would be to determine and redirect in static.py.

We should avoid depending on our Python server, as ultimately I want to decouple serving the frontend client from the backend server. We already do this decoupling with the preview builds, which are served 100% by Netlify.

So we're left with 1 & 3, and I think option 3 is the most robust option.

arikfr · 2019-03-20T14:50:14Z

Eslint rule - error on unsupported browser feature usage 👮

If we only use browser supported features, what does Babel transpile?

ranbena · 2019-03-20T15:07:50Z

If we only use browser supported features, what does Babel transpile?

It's a good question and I'd like to know as well. My understanding is that it does not polyfill all features and if a feature is polyfilled (either with dedicated babel plugin / npm package / own util) we need to be explicitly define it to be excluded from the linter.

For instance, if we'd still be targeting IE we'd get a lint error on using Promise but could polyfill it with dedicated package.

ranbena · 2019-03-21T06:17:27Z

What I want to do

Generate browserslist-useragent.js file that suits old browsers to be loaded in client/app/index.html.

The plan

Define an npm script unsupported:build which transpiles the browserslist-useragent npm package with a dedicated babel config into a single file.

@arikfr

Does this make sense?
Can babel cli transpile a node module with its dependencies? Or can webpack be used to so that?
How do you provide babel with a preset override? (e.g. targets: [ "ie": 8 ])

arikfr · 2019-03-21T06:54:26Z

This is what webpack is for. Not sure about the overrides but we can have a dedicated configuration for it (maybe in a separate project?). But I feel like we’re reinventing the wheel here. Is there some prior art on how this is done?

…

-- Arik Fraimovich

On Thu, Mar 21 2019 at 8:17 AM, < ***@***.*** > wrote: What I want to do Generate browserslist-useragent.js file that suits old browsers to be loaded in client/app/index.html. The plan Define an npm script unsupported:build which transpiles the browserslist-useragent npm package with a dedicated babel config into a single file. @arikfr ( https://github.com/arikfr ) * Does this make sense? * Can babel cli transpile a node module with its dependencies? Or can webpack be used to so that? * How do you provide babel with a preset override? (e.g. targets: [ "ie": 8 ] ) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub ( #3609 (comment) ) , or mute the thread ( https://github.com/notifications/unsubscribe-auth/AAEXLEfM_JgT52s9oyVnV6WZfUajfo6Nks5vYyP6gaJpZM4b9QQl ).

ranbena · 2019-03-21T08:34:03Z

Tested the redirect method in IE10 and 11 but the rest later this week (with windows machine).

ranbena · 2019-03-21T08:35:03Z

+        onIE();
+      }
+    </script>
+    <!--[if IE]><script>onIE()</script><![endif]-->


IE only for now, consider using https://github.com/browserslist/browserslist-useragent in the future.

ranbena · 2019-03-26T05:47:12Z

@arikfr made html page static, no template. Lmk if this is what you meant.

ranbena · 2019-03-26T06:48:58Z

Damnit, another codeclimate eslint plugin problem..
Might have to do with eslint channel compatibility 😒
@arikfr any reason not to switch to CircleCI step instead (assuming it's problem free)?

ranbena · 2019-03-26T08:02:39Z

Also, @arikfr I put the redirect script in index.html and multi_org.html (dunno what that is).
Should I put it in signed_out.html (a main template) as well?

ranbena · 2019-03-26T17:26:06Z

~~@arikfr I don't get the pep8 error here - there are actually 2 lines..~~

arikfr · 2019-03-26T17:44:29Z

-  }
+  },
+  "//": "browserslist set to 'Async functions' compatibility",
+  "browserslist": [


How about using configuration like we had before (> 0.5%, last 2 versions, Firefox ESR, ie 11, not dead) so we don't have to update this list everytime browser versions update? I mean not necessarily the same one, but along the lines of > 0.5%, last 2 versions etc instead of hardcoded versions.

so we don't have to update this list every time browser versions update

Why would you need to? We should revisit this list when:

There are lower browser versions we would like to support. Won't happen cause we've looked at the browser stats.

As a performance optimization to lower app.js file size. That can be done periodically.

instead of hardcoded version

The advantage of hardcoded versions is that they allow to easily and reliably determine if a user's browser is supported or not. But if we define sth like "last 2 versions" you would have no clear cutoff point (cause under last 2 versions doesn't mean you can't use Redash).

arikfr · 2019-03-27T15:47:27Z

👍

* Browser support config * Removed some offending code * Added unsupported html page and redirect for IE * Typo in regex * Made html page static * Added redirect script to multi_org * Moved static html page to client/app

ghost assigned ranbena Mar 19, 2019

ghost added the in progress label Mar 19, 2019

ranbena requested review from arikfr and kravets-levko March 19, 2019 22:34

ranbena commented Mar 19, 2019

View reviewed changes

ranbena mentioned this pull request Mar 20, 2019

Nest query ACL to dropdowns #3544

Merged

1 task

ranbena force-pushed the browser branch from 657942d to 0752d57 Compare March 21, 2019 08:06

ranbena marked this pull request as ready for review March 21, 2019 08:25

ranbena commented Mar 21, 2019

View reviewed changes

ranbena changed the title ~~Browser support config (wip)~~ Browser support config Mar 21, 2019

ranbena force-pushed the browser branch from c156554 to 56df0aa Compare March 26, 2019 06:13

ranbena force-pushed the browser branch from 56df0aa to 659c032 Compare March 26, 2019 15:03

arikfr reviewed Mar 26, 2019

View reviewed changes

Comment thread redash/templates/unsupported.html

ranbena force-pushed the browser branch from c3ee6b8 to 3d114d6 Compare March 26, 2019 19:34

ranbena added 3 commits March 27, 2019 07:12

Browser support config

337f8a5

Removed some offending code

9dd7d18

Added unsupported html page and redirect for IE

3336b9d

ranbena added 4 commits March 27, 2019 07:12

Typo in regex

c543b5a

Made html page static

d9f61a6

Added redirect script to multi_org

56d7a01

Moved static html page to client/app

52dafb3

ranbena force-pushed the browser branch from 3d114d6 to 52dafb3 Compare March 27, 2019 05:12

arikfr merged commit b5d97e2 into master Mar 27, 2019

arikfr deleted the browser branch March 27, 2019 15:47

arikfr removed the in progress label Mar 27, 2019

jezdez mentioned this pull request May 21, 2019

Content Security Policy error because of inline script #3816

Closed

Uh oh!

Conversation

ranbena commented Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kravets-levko Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranbena Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arikfr commented Mar 20, 2019 via email

Uh oh!

ranbena commented Mar 20, 2019

Uh oh!

arikfr commented Mar 20, 2019

Uh oh!

arikfr commented Mar 20, 2019

Uh oh!

ranbena commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ranbena commented Mar 21, 2019

What I want to do

The plan

Uh oh!

arikfr commented Mar 21, 2019 via email

Uh oh!

ranbena commented Mar 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranbena commented Mar 26, 2019

Uh oh!

ranbena commented Mar 26, 2019

Uh oh!

ranbena commented Mar 26, 2019

Uh oh!

ranbena commented Mar 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arikfr commented Mar 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ranbena commented Mar 19, 2019 •

edited

Loading

kravets-levko Mar 20, 2019 •

edited

Loading

ranbena Mar 20, 2019 •

edited

Loading

ranbena commented Mar 20, 2019 •

edited

Loading

ranbena commented Mar 26, 2019 •

edited

Loading