Lazy load babel on REPL #1318

existentialism · 2017-08-15T16:39:45Z

EDIT:

Reduced it to a simple version string output:

existentialism · 2017-08-15T16:40:20Z

js/repl/src/ReplOptions.js

-    overflow: 'auto',
-    boxSshadow:
+    flexDirection: 'column',
+    boxShadow:


@bvaughn found a typo we all missed :)

🤡 It's not widely know, but the box-sshadow is awesome

babel-bot · 2017-08-15T16:43:00Z

Deploy preview failed.

Built with commit ece0494

https://app.netlify.com/sites/babel/deploys/599b45217960b1269842291f

existentialism · 2017-08-15T16:43:39Z

js/repl/src/Repl.js

@@ -160,6 +162,27 @@ export default class Repl extends React.Component {
    );
  }

+  _setupBabel() {
+    loadBabel(this.state.babel, success => {


We could refactor this alongside loadPlugins to be a bit more generic, but figured we could do that in a follow up, if anything.

bvaughn · 2017-08-15T20:04:33Z

Couple of quick thoughts:

Haven't reviewed the code yet, but the UI looks nice. 😄
The new label eats up a lot of space- especially for the responsive layout. I wonder if we can hide it by default somehow? (I'd be happy to help with this if you'd like!)
Passing an invalid build number causes things to hang for a long time waiting on a 500 response from circleci.com/api/v1.1/project/github//<invalidid>/artifacts and then we don't handle the error. (Maybe this is okay and it's not worth supporting?)

hzoo · 2017-08-15T20:07:54Z

I think it's fine for the invalid build number since you would be going to the link via babel-bot not manually (you shouldn't be at least), make the 99% case fast (also circle ci isn't doing so hot atm https://status.circleci.com/)

hzoo · 2017-08-15T20:08:39Z

Would be nice to link to the corresponding PR, or github release

bvaughn · 2017-08-15T20:22:14Z

Maybe we should hide this label unless the URL flag is specified? This way we don't eat up so much space.

existentialism · 2017-08-15T21:56:55Z

@bvaughn yeah, I'm still experimenting with other options. I think it's valuable to show the current Babel version at somewhere, though I agree it's currently taking up a bit more space than we'd like.

bvaughn · 2017-08-15T22:18:52Z

I was kind of picturing an info/question icon that's either FAB-style or maybe shown only on hover.

Daniel15 · 2017-08-16T17:20:20Z

js/repl/src/loadBabel.js

+
+  // No specific version passed, so just download the latest release.
+  if (!version) {
+    version = 'next'; // This should be changed to "latest" when Babel 7 is stable


Maybe this should actually be 6 since this is the stable REPL.

Daniel15 · 2017-08-16T17:21:56Z

js/repl/src/loadBuildArtifacts.js

+    }
+
+    const artifacts = response.filter(x =>
+      /babel-standalone\/babel.js$/.test(x.path)


You have way more newlines than I did in my version... were my lines too long? 😛

I ran prettier :)

Prettier ftw 😁

Prettier is SO NICE. I love it. We ran it across millions of lines of code at work.

Daniel15 · 2017-08-16T17:23:34Z

js/repl/src/loadBuildArtifacts.js

+  xhr.onload = function() {
+    const response = JSON.parse(xhr.responseText);
+    if (response.message) {
+      alert(`Could not load Babel build #${build}: ${response.message}`);


I wanted to display these in a nicer way, but there was no good way to get the error message into the UI. Maybe we could improve that now with the new REPL?

Daniel15 · 2017-08-16T17:28:10Z

The "repo" (Daniel15/babel in your example) is going to be very very rarely used, so I wouldn't bother showing it in the UI. Essentially, it will only differ if the PR is going to a different repo (just for testing purposes). For example, to test out CircleCI builds, I sent a PR from Daniel15Test/babel to Daniel15/babel. For any PRs to the "real" Babel, it'll always be babel/babel as that's the CircleCI project that's being built.

For CircleCI builds, we append the PR number to the version number: https://github.com/babel/babel/blob/7.0/packages/babel-standalone/src/gulpTasks.js#L19-L20. It'll look like 6.24.0+pr.12345 or 7.0.0-alpha.15+pr.12345 where 12345 is the PR number. Given that, we probably don't need to show the build number in the UI. Although we could have multiple builds per PR (every time the user pushes to the branch, we'll build it again) so maybe it's beneficial to show the build number. Hmm.

existentialism · 2017-08-18T16:11:36Z

@bvaughn @Daniel15 rebased/updated... basically reduced this to only output the version number.

We can probably unify the "Loading..." from the Jekyll template with the initial "Loading Babel..." state in a further PR.

bvaughn · 2017-08-18T16:14:17Z

I like the loading UI! 😄

I'm still not a fan of showing the version by default though. (I think it takes up too much space and adds too little value.) I think we should only show the version label if the user has specified a target version in the URL. That is, unless we can make it more subtle.

Daniel15 · 2017-08-18T16:15:49Z

I like a showing the version. It makes it obvious which version is being used (for example, if there's an issue with babel-standalone and it doesn't publish correctly, we'll see that there's an older version in use). Sent from my phone.

…

On Aug 18, 2017 9:14 AM, "Brian Vaughn" ***@***.***> wrote: I like the loading UI! 😄 I'm still not a fan of showing the version by default though. I think we should only show the version label if the user has specified a target version in the URL. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1318 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFnHU-oiSg6g_3THkg8inU9TSJmbtJLks5sZbhagaJpZM4O30cb> .

bvaughn · 2017-08-18T16:18:05Z

for example, if there's an issue with babel-standalone and it doesn't publish correctly, we'll see that there's an older version in use

Do we really need the REPL UI to detect this case? 🤔

Sidebar space is precious, especially as we expand to add more options there (eg 3rd party plugins from NPM). I can see us needing to switch to an accordion UI in the near future to make more room for things.

hzoo · 2017-08-18T16:18:39Z

Knowing the version of babel-core is important if we want to use the repl as a way to report issues (ideally we'd have every version in some menu then I guess)

existentialism · 2017-08-18T16:20:12Z

One thing we could do is have the debug option always available (and not just for env's debug output), and output the version info there?

bvaughn · 2017-08-18T16:21:19Z

Sounds like I'm in the minority here then 😄 in which case, that's cool. I'll defer.

However, I still think we should at least change the visual treatment of it to be a part of the rest of the sidebar rather than being a sticky footer part. Making it sticky forces it to take up more space (since you can't even scroll it off screen if your screen is smaller.)

bvaughn · 2017-08-18T19:38:41Z

I was going to take a quick stab at tweaking the version label but on this branch, make serve just hangs for me. Switching back to master works fine though.

Wondering if anyone else has seen this?

existentialism · 2017-08-18T19:41:33Z

@bvaughn jekyll hates you :) I have a fix for making the label part of the sidebar, will push in a sec

bvaughn · 2017-08-18T19:44:59Z

jekyll hates you

Yeah maybe so...but I think it's worth confirming with at least one other person before merging.

I have a fix for making the label part of the sidebar, will push in a sec

Great 👍

Daniel15 · 2017-08-18T19:46:29Z

I think it's worth confirming with at least one other person before merging.

Travis and Netlify are both OK with this diff... But they're robots so maybe they don't count as "other people" 😛

Also they're building the site (make build) not watching. Could you try make build and see if that works for you?

bvaughn · 2017-08-18T19:51:35Z

Pfft 😆 Robots!

Yes, make build works for me. make serve hangs (although it's pretty speedy on master).

$ make serve
bundle check || bundle install; \
	bundle exec jekyll serve
The Gemfile's dependencies are satisfied
Configuration file: /Users/bvaughn/Documents/git/babel.github.io/_config.yml
Configuration file: /Users/bvaughn/Documents/git/babel.github.io/_config.yml
            Source: /Users/bvaughn/Documents/git/babel.github.io
       Destination: /Users/bvaughn/Documents/git/babel.github.io/_site
 Incremental build: disabled. Enable with --incremental
      Generating...

Compare to master:

$ make serve
# Run both Jekyll and Webpack concurrently.
make -j2 serve-jekyll serve-webpack
./node_modules/.bin/webpack-dev-server
bundle check || bundle install; \
	bundle exec jekyll serve --incremental
The Gemfile's dependencies are satisfied
Project is running at http://localhost:34512/
webpack output is served from http://localhost:34512/
Configuration file: /Users/bvaughn/Documents/git/babel.github.io/_config.yml
            Source: /Users/bvaughn/Documents/git/babel.github.io
       Destination: /Users/bvaughn/Documents/git/babel.github.io/_site
 Incremental build: enabled
      Generating...
Hash: d13355e738e68f810938
Version: webpack 3.5.4
Time: 2144ms

Looks like this branch isn't kicking off webpack-dev-server?

Daniel15 · 2017-08-18T19:53:44Z

Hmm that's very weird. make build and make serve should be near-identical except serve also watches for changes.

I could try it on my computer but it won't be until tonight - All my Babel stuff is on my home PC rather than my work PC.

bvaughn · 2017-08-20T15:34:25Z

Fetched and pulled and make serve is behaving now.

bvaughn

Looks nice! Left a few minor things.

bvaughn · 2017-08-20T15:36:03Z

js/repl/PresetLoadingAnimation.js

+  className: ?string,
+};
+
+const PresetLoadingAnimation = ({ className }: PresetLoadingAnimationProps) =>


subjective nit: Named param inline defaults are nice too for element props 😄

const PresetLoadingAnimation = ({ className = "" }: PresetLoadingAnimationProps) =>

bvaughn · 2017-08-20T15:44:13Z

js/repl/Repl.js

+          } else {
+            babelState.didError = true;
+            babelState.isLoading = false;
+          }


Mutable data in state is kind of a sketchy part of my original design here, but I didn't want the added weight of pulling in a lib like Immutable. That being said-

Since you're just mutating state.babel in this callback, and you pass state.babel to loadBabel - why not just have it wrap the callback and update the isLoaded and isLoading values before calling it? (That would be more consistent with what loadPlugin does.)

bvaughn · 2017-08-20T15:46:17Z

js/repl/ReplOptions.js

    minWidth: "150px",
    display: "flex",
    overflow: "auto",
-    boxSshadow:
+    boxShadow:


bvaughn · 2017-08-20T15:46:30Z

js/repl/ReplOptions.js

+    paddingRight: "1.5rem",
+
+    [media.large]: {
+      borderTop: "2px solid #121417",


Subjective nit: Not a big fan of the top border. Wondering if you considered a different background color instead, to make this section stand out?

background-color: #181a1f;

bvaughn · 2017-08-20T15:48:58Z

js/repl/UriUtils.js

      "evaluate",
      "lineWrap",
      "presets",
      "prettier",
      "showSidebar",
      "targets",
+      "targets",


Duplicate "targets" ^

bvaughn · 2017-08-20T15:50:10Z

js/repl/loadBabel.js

+
+  // See if a CircleCI build number was passed in the path
+  // Prod (with URL rewriting): /repl/build/12345/
+  // Dev: /repl/#?build=12345


Great comment!

Heh, this is my comment (this is copied across from 7.html). I try to keep them informative. 😛

bvaughn · 2017-08-20T15:52:47Z

js/repl/loadBuildArtifacts.js

+    if (!artifacts || artifacts.length === 0) {
+      alert(
+        `Could not find valid babel-standalone artifact in build #${build}`
+      );


Why alert? Shouldn't we invoke the callback with a false-success so our error state is consistent?

Was originally trying to get the first version of this as a straight port of @Daniel15's original PR, but I'll go ahead bubble up the error through the callbacks and show it in the "Loading Babel" message.

bvaughn · 2017-08-20T15:53:00Z

js/repl/loadBuildArtifacts.js

+  };
+
+  xhr.onerror = function() {
+    alert(`Could not load Babel build #${build} :(`);


Ditto as above.

bvaughn · 2017-08-20T15:56:45Z

js/repl/loadBuildArtifacts.js

@@ -0,0 +1,39 @@
+export default function loadBuildArtifacts(
+  repo: string = "babel/babel",


null or empty string "" will override default params, so I don't think this will actually work. (If no circleciRepo query parameter is specified, an empty string gets passed to this function- since that's the default replUtils sets.)

existentialism · 2017-08-21T15:49:28Z

@bvaughn thanks for the review, made the changes and now error messages bubble up to the loading screen; can follow up this PR with improved versions (tweak copy, can detect 404s on builds, probably add an error icon instead of sad face emoji, etc).

bvaughn · 2017-08-21T16:23:31Z

add an error icon instead of sad face emoji

Love it 😁 Happy to help with this

bvaughn · 2017-08-21T16:23:55Z

js/repl/PresetLoadingAnimation.js

+const PresetLoadingAnimation = ({
+  className = "",
+}: PresetLoadingAnimationProps) =>
+  <div className={`${className || ""} ${styles.loadingAnimation}`}>


nit: className || "" is redundant with the initializer above

bvaughn · 2017-08-21T16:24:54Z

js/repl/Repl.js

+    if (!state.babel.isLoaded) {
+      let message = "Loading Babel...";
+
+      if (!state.babel.isLoading && state.babel.didError) {


nit: state.babel.didError should be sufficient?

bvaughn · 2017-08-21T16:25:25Z

js/repl/Repl.js

+      }));
+
+      if (babelState.isLoaded) {
+        this._checkForUnloadedPlugins();


Should this be done in the setState callback?

Daniel15 · 2017-08-22T04:15:42Z

js/repl/loadBuildArtifacts.js

+      }
+    }
+
+    cb(url, error);


Would be good to use promises here, but then I guess we'd need to load a polyfill (maybe?)

Daniel15

This looks good! shipit!

existentialism commented Aug 15, 2017

View reviewed changes

existentialism force-pushed the issue1313 branch from e262b33 to cf7fe40 Compare August 15, 2017 16:42

existentialism commented Aug 15, 2017

View reviewed changes

Daniel15 reviewed Aug 16, 2017

View reviewed changes

existentialism force-pushed the issue1313 branch from cf7fe40 to 5b0b58f Compare August 18, 2017 16:08

bvaughn mentioned this pull request Aug 18, 2017

Nicer loading state for new REPL #1325

Closed

existentialism force-pushed the issue1313 branch from d861c70 to e95414e Compare August 19, 2017 16:06

existentialism assigned bvaughn and Daniel15 Aug 20, 2017

bvaughn reviewed Aug 20, 2017

View reviewed changes

bvaughn mentioned this pull request Aug 21, 2017

Investigate moving compilation to web-worker #1342

Closed

existentialism added 2 commits August 21, 2017 10:43

Lazy load Babel on REPL

59e0eac

review fixes and bubble up error message

848320b

existentialism force-pushed the issue1313 branch from e95414e to 848320b Compare August 21, 2017 15:44

change bg of version on desktop

26f1c36

bvaughn reviewed Aug 21, 2017

View reviewed changes

bvaughn approved these changes Aug 21, 2017

View reviewed changes

fixes

ece0494

Daniel15 reviewed Aug 22, 2017

View reviewed changes

Daniel15 approved these changes Aug 22, 2017

View reviewed changes

existentialism merged commit 55454fd into master Aug 22, 2017

existentialism deleted the issue1313 branch August 22, 2017 04:21

		@@ -0,0 +1,39 @@
		export default function loadBuildArtifacts(
		repo: string = "babel/babel",

Lazy load babel on REPL #1318

Lazy load babel on REPL #1318

Conversation

existentialism commented Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

babel-bot commented Aug 15, 2017 • edited Loading

existentialism Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

bvaughn commented Aug 15, 2017

hzoo commented Aug 15, 2017

hzoo commented Aug 15, 2017

bvaughn commented Aug 15, 2017

existentialism commented Aug 15, 2017

bvaughn commented Aug 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daniel15 commented Aug 16, 2017 • edited Loading

existentialism commented Aug 18, 2017 • edited Loading

bvaughn commented Aug 18, 2017 • edited Loading

Daniel15 commented Aug 18, 2017 via email

bvaughn commented Aug 18, 2017

hzoo commented Aug 18, 2017

existentialism commented Aug 18, 2017

bvaughn commented Aug 18, 2017 • edited Loading

bvaughn commented Aug 18, 2017 • edited Loading

existentialism commented Aug 18, 2017

bvaughn commented Aug 18, 2017

Daniel15 commented Aug 18, 2017

bvaughn commented Aug 18, 2017 • edited Loading

Daniel15 commented Aug 18, 2017

bvaughn commented Aug 20, 2017

bvaughn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

existentialism commented Aug 21, 2017

bvaughn commented Aug 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daniel15 left a comment

Choose a reason for hiding this comment

existentialism commented Aug 15, 2017 •

edited

Loading

babel-bot commented Aug 15, 2017 •

edited

Loading

existentialism Aug 15, 2017 •

edited

Loading

Daniel15 commented Aug 16, 2017 •

edited

Loading

existentialism commented Aug 18, 2017 •

edited

Loading

bvaughn commented Aug 18, 2017 •

edited

Loading

bvaughn commented Aug 18, 2017 •

edited

Loading

bvaughn commented Aug 18, 2017 •

edited

Loading

bvaughn commented Aug 18, 2017 •

edited

Loading