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

Import ES5 autotrack.js #203

Merged
merged 1 commit into from Feb 26, 2019
Merged

Import ES5 autotrack.js #203

merged 1 commit into from Feb 26, 2019

Conversation

woeltjen
Copy link
Contributor

@woeltjen woeltjen commented Feb 26, 2019

...to avoid SyntaxError in legacy browsers

googleanalytics/autotrack#137

Description

Reports of Uncaught SyntaxError: Use of const in strict mode when viewing Beenest on legacy browsers (Safari 9/10 were the reports, Chromium 40 was any reproduction environment). Generally, these occur when trying to run ES6 code in a browser without ES6 support, such as when an ES6 dependency gets included by webpack without transpilation. In our case, this was the autotrack dependency. Ran into miscellaneous errors trying to enable transpilation for node_modules/autotrack; instead, changed the require call which imports this to point to autotrack/autotrack.js, which is a minified ES5 version of the dependency.

How to Test

  1. Build for prod (npm run prod)
  2. Serve built file (cd dist; http-server)
  3. Verify that Chromium 40 can load the page

Which devices did you test on?

  • Chromium on Mac
  • Chrome on Mac
  • Chrome on PC
  • Firefox on Mac
  • Firefox on PC
  • Safari iPhone
  • Chrome Android

REVIEWERS:

Check against these principles:

High level

Does this code need to be written?
What are the alternatives?
Will this implementation become a support issue?
How much error margin does this solution have?

Code

Variables/Naming:

  • Would the variable type led to future edge cases?
  • Are the variable naming clear? Would the value contain something other than what the name describes.

Security

  • Can this be hacked or abused by the user?

Further Work

This solution is not ideal, insofar as it increases the scope of the dependency we include; fine-grained inclusion of plugins would be preferable. Our index.js goes from 1575840 bytes to 1582801 bytes with this change. That extra ~7k looks "negligible" at that scale; we should probably look into pruning our other dependencies.

Learnings

webpack can introduce some obscure errors...

...to avoid SyntaxError in legacy browsers

googleanalytics/autotrack#137
@kcvan
Copy link
Contributor

kcvan commented Feb 26, 2019

what

@tc tc merged commit b41ddd7 into master Feb 26, 2019
@tc tc deleted the vic/ENG-1032 branch February 26, 2019 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants