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

Solve babeling issue highlighted by react-amphtml #2181

Closed
dr3 opened this issue Jun 28, 2019 · 4 comments
Closed

Solve babeling issue highlighted by react-amphtml #2181

dr3 opened this issue Jun 28, 2019 · 4 comments
Labels
investigation Refinement Needed This is ready for refinement. It shouldn't be worked on until it has been refined by Dev & Test.
Milestone

Comments

@dr3
Copy link
Contributor

dr3 commented Jun 28, 2019

Is your feature request related to a problem? Please describe.
The package react-amphtml above v1.0.1 has an arrow function in, which when used in simorgh doesn't get babeled and makes IE 11 break

Attempt at fix #1897

Describe the solution you'd like
Come up with a solution for addressing this problem. 1 possible fix is to babel our node modules, however this is often discouraged online, so read up.

Another option is to babel individual packages on required basis, would require method of detecting these modules

Describe alternatives you've considered
Just ignoring it, starting a new life on a farm somewhere, raising sheep, relaxing.

Testing notes
This is an investigation issue.

Dev insight: Will Cypress tests be required or are unit tests sufficient? Will there be any potential regression? etc

Additional context
Add any other context or screenshots about the feature request here.

@dr3 dr3 added Refinement Needed This is ready for refinement. It shouldn't be worked on until it has been refined by Dev & Test. simorgh-core-stream labels Jun 28, 2019
@dr3 dr3 added this to To do in Simorgh via automation Jun 28, 2019
@jamesbhobbs
Copy link
Contributor

I suggest this babel/babel-loader#171 (comment) but we'd do an include rather than exclude.

@dfrankland
Copy link

Hello there! While making react-amphtml I didn't expect it to be used outside of Node.js, which is why I have Babel target Node.js version 8+. I'd happily add a distributable made for the browser if you'd like 👍 Would it be okay of me to ask what you are targeting in terms of browser support?

@dr3
Copy link
Contributor Author

dr3 commented Jun 30, 2019

Hey @dfrankland! It makes sense to just target node 8, the way were currently using your package just highlighted this problem for us 🙈 Our app shares code on the client and server, which atm means a bunch of AMP code (and deps), which are only used on the server, get pulled into the client bundles.

We're targeting back to IE, but I think going forward we should avoid pulling in server only deps like react-amphtml to the client 😂 Thanks for offering to add a browser distributable, hopefully we will fix our issue soon to avoid needing it though 🤞😆

I think this issue will be split into "add process for selective dep babeling" and "remove amp logic from client bundles"

@jamesbhobbs jamesbhobbs added this to the Simorgh 3.0 milestone Aug 20, 2019
@dr3
Copy link
Contributor Author

dr3 commented Oct 14, 2019

For refinement; this issue is 80% investigations and research. Whoever completes it should have a collection of links to online blogs etc justifying why/why they arnt doing stuff :) we should follow best practices here :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
investigation Refinement Needed This is ready for refinement. It shouldn't be worked on until it has been refined by Dev & Test.
Projects
None yet
Development

No branches or pull requests

6 participants