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

update babel config to target newer browsers, work around parcel bug #60

Merged
merged 3 commits into from
Apr 16, 2018

Conversation

tafsiri
Copy link
Contributor

@tafsiri tafsiri commented Apr 12, 2018

This does a few things.

It changes the build target for all the examples to support the following newer browsers currently,

and_chr 64
and_uc 11.8
chrome 64
chrome 63
firefox 58
ios_saf 11.0-11.2

The exception to this is getting_started, which still doesn't use a build tool, and webcam-transfer-learning. The reason for the latter is because that demo is featured on our front page it is more likely to be viewed by non-developers, who may have older browsers.

Targetting newer browsers makes the stack traces easier to parse as mentioned in tensorflow/tfjs#60

The other thing this PR does is workaround a bug in parcel that was often causing an error to happen in the dev server and require a restart of yarn. Moving to .babelrc files should fix the problem and allow for much smoother development.

I also updated the readme to spell out the dependencies needed, which should close tensorflow/tfjs#57

This one might be nice for people to try out locally.


This change is Reviewable

@dsmilkov
Copy link
Contributor

:lgtm_strong: couple small comments


Reviewed 24 of 24 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


README.md, line 13 at r1 (raw file):

Except for `getting_started`, all the examples require the following dependencies to be installed.

 - Node.js version 8.9 or higher

curious, where does 8.9 come from? Is that parcel's requirement?


mnist-core/dist-orig/index.html, line 1 at r1 (raw file):

<!--

shouldn't this file live directly under mnist-core and not in a dist-orig folder?


Comments from Reviewable

@tafsiri
Copy link
Contributor Author

tafsiri commented Apr 16, 2018

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


README.md, line 13 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

curious, where does 8.9 come from? Is that parcel's requirement?

Yep this was working around a bug in parcel on older versions of node. 8.9 is the first LTS release of node after node 6 (the prev LTS release)


mnist-core/dist-orig/index.html, line 1 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

shouldn't this file live directly under mnist-core and not in a dist-orig folder?

Good catch. this file should not even be here. I was diffing against the new output. will delete.


Comments from Reviewable

@nsthorat
Copy link

:lgtm_strong:


Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


Comments from Reviewable

@tafsiri tafsiri merged commit f34f6df into master Apr 16, 2018
@dsmilkov dsmilkov deleted the compile-target branch May 6, 2018 04:52
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.

A list of prereqs could be handy
3 participants