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

Fix browser-support related bugs #1

Merged
merged 1 commit into from
Jan 14, 2019
Merged

Conversation

TerrenceLJones
Copy link
Contributor

@ItsJonQ I think I've narrowed down the problem to two different issues - both of which occur in their own way on IE11.

Problem 1

When running the project locally the following errors are being thrown in the console
screen shot 2019-01-12 at 11 50 44 pm

Background

Create React App(CRA) does not allow for overriding config files like babel.rc. So if the user runs npm run start which calls react-scripts start, CRA doesn't read the local Babel file and the @helpscout/zero/babel preset is not being consumed.

How to reproduce

  • Start up local dev server npm run start
  • Navigate to BrowserStack and load IE11
  • Open the console then navigate to the dev servers url
  • Note the error shown in the console. If you navigate to the line in the code that is error via the console you'll notice that the output code is ES6 object property value shorthand instead of ES5 code and that Promise is undefined errors are also being thrown

How to fix

  • Install 'react-app-polyfill/ie11’ and require at the top of index.js file
  • (Another possible, but undesirable fix, for this would be to eject from CRA and maintain configurations ourselves then local config files could
    be updated. But this seems to be what the kcd-scripts dependency (mentioned below) is helping to alleviate the need for. So the first suggested fix seems like the best option. )

Problem 2

When running a build of Beacon DevTools the built files are not being compiled correctly for required browsers - i.e. IE11.

Background

One of the Beacon DevTools' dependencies, Zero, inherited a requirement (or quirk 😉) from its inspiration kcd-scripts (also a dependency of Zero). Inside of zero/src/config/babelrc.js this line https://github.com/helpscout/zero/blob/master/src/config/babelrc.js#L38 is loading the declared browserslist from Beacon DevTools' package.json:

"browserslist": [
    ">0.2%",
    "not dead",
    "not ie <= 11",
    "not op_mini all"
  ],

The problem (quirk) is occurring on this line: https://github.com/helpscout/zero/blob/master/src/config/babelrc.js#L43. For the browsersConfig to be consumed it is required that one of two environment variables are set at runtime: Either BUILD_WEBPACK or BUILD_ROLLUP. Because this is not currently occurring, the build target is node which is not compiling correctly to support required browsers.

How to reproduce

How to reproduction:

  • run npm run build
  • Look in dist/actions/index.js
  • Note that (approx) line 19 is not being compiled from the ES6 object property shorthand to the ES5 version (If I remember correctly this is the line that is breaking in Beacon 2.1 when running the app in IE11)

How to fix

  • Update the build task to BUILD_WEBPACK=true zero build

@TerrenceLJones
Copy link
Contributor Author

@ItsJonQ
Copy link
Contributor

ItsJonQ commented Jan 14, 2019

@TerrenceLJones This is awesome! Thank you so much for the thorough investigation + fixes!

@ItsJonQ ItsJonQ added the bug Something isn't working label Jan 14, 2019
Copy link
Contributor

@ItsJonQ ItsJonQ left a comment

Choose a reason for hiding this comment

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

@TerrenceLJones Tested in Browserstack. Works great!!!

@ItsJonQ ItsJonQ merged commit 0bde67d into master Jan 14, 2019
@TerrenceLJones
Copy link
Contributor Author

🎉 woo hoo!

@TerrenceLJones
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants