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.

Sign up for GitHub

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

Jump to bottom

Improve browsercheck script #4839

Merged
merged 7 commits into from
Dec 30, 2019
Merged

Improve browsercheck script #4839

merged 7 commits into from
Dec 30, 2019

Conversation

bhollis
Copy link
Contributor

@bhollis bhollis commented Dec 30, 2019

  1. Automatically accept newer versions of the browsers we accept (e.g. if we accept the latest two versions of Chrome, which are 78 and 79, we'll still accept 80 and 81).
  2. Link to a new support wiki about which browsers we support.
  3. Remove the runtime dependency on browserslist, greatly reducing the size of this script. I realized I could just compute it as part of the build.
  4. Force the script's dependencies to be bundled with it, so it doesn't split into multiple chunks. This helps it continue to run even if the rest of the webpack entry points fail.
  5. Avoid running babel on browserscheck - it's meant to be written as maximally-compatible ES5 already and we don't want to inject core-js polyfills.
  6. Remove "unreleased versions" which avoids this bug in babel-preset-env.

I may just want to switch to https://github.com/ElemeFE/obsolete-webpack-plugin later, since it seems like it's just a better (if much more complicated) version of this, but for now these changes help.

@bhollis bhollis mentioned this pull request Dec 30, 2019
Closed
@bhollis bhollis requested a review from delphiactual December 30, 2019 05:22
@bhollis
Copy link
Contributor Author

bhollis commented Dec 30, 2019

Well that's disappointing. I thought I was getting around the babel bug. We may have to just wait for the fix.

@archived-2f4dd0
Copy link
Contributor

i'm getting a new-to-me "Compiled with warnings." which i assume you know,
and in firefox 71 (current afaik) a not-supported warning
chrome seems fine

@archived-2f4dd0
Copy link
Contributor

nevermind the js UA is
"Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:72.0) Gecko/20100101 Firefox/72.0"

@bhollis
Copy link
Contributor Author

bhollis commented Dec 30, 2019

Cool, I'll test it out. I don't know what the "Compiled with warnings" thing is - I'm not seeing any warnings in the CLI output.

@bhollis
Copy link
Contributor Author

bhollis commented Dec 30, 2019

Er, I guess I am seeing the warnings, but I don't know what they are. The toaster says something about immer (which we recently upgraded) but I don't see the full output.

@archived-2f4dd0
Copy link
Contributor

same yeah it's just toasting :|

@bhollis
Copy link
Contributor Author

bhollis commented Dec 30, 2019 

WARNING in ./node_modules/immer/dist/immer.module.js
Module Warning (from ./node_modules/source-map-loader/index.js):
(Emitted value instead of an instance of Error) Cannot find source file '../node_modules/tslib/tslib.es6.js': Error: Can't resolve '../node_modules/tslib/tslib.es6.js' in '/Users/brh/Documents/oss/DIM/node_modules/immer/dist'
 @ ./src/app/item-review/reducer.ts 10:0-28 46:15-22 67:15-22 85:15-22
 @ ./src/Index.tsx

🤷‍♂

@bhollis
Copy link
Contributor Author

bhollis commented Dec 30, 2019

immerjs/immer#487

@bhollis
Copy link
Contributor Author

bhollis commented Dec 30, 2019

OK I added some Jest tests - feel free to throw more user agent strings at me.

delphiactual
delphiactual approved these changes Dec 30, 2019
View reviewed changes
src/browsercheck.js
var version = (browserName === 'ios_saf'
? agent.os.version
: agent.browser.version || agent.os.version || ''
).split('.');
while (version.length > 0) {
try {
return browserslist(browserName + ' ' + version.join('.'))[0];
return browserName + ' ' + version.join('.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return browserName + ' ' + version.join('.');
return `${browserName} ${version.join('.')}`;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this file, which needs to stay es5-compatible.

src/browsercheck.js
// Detect anything based on chrome as if it were chrome
var chromeMatch = /Chrome\/(\d+)/.exec(agent.ua);
if (chromeMatch) {
browser = 'chrome ' + chromeMatch[1];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
browser = 'chrome ' + chromeMatch[1];
browser = `chrome ${chromeMatch[1]}`

src/browsercheck.js
}
if (!supported) {
console.warn(
'Browser ' + browser + ' is not supported by DIM. Supported browsers:',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'Browser ' + browser + ' is not supported by DIM. Supported browsers:',
`Browser ${browser} is not supported by DIM. Supported browsers:`,

@bhollis bhollis merged commit 8aff9e5 into master Dec 30, 2019
@bhollis bhollis deleted the browsercheck branch December 30, 2019 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@delphiactual delphiactual delphiactual approved these changes

@archived-2f4dd0 archived-2f4dd0 Awaiting requested review from archived-2f4dd0

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bhollis @archived-2f4dd0 @delphiactual