Skip to content

[REPL] Allow loading latest build from specified branch#1356

Merged
Daniel15 merged 2 commits intobabel:masterfrom
Daniel15:repl-branch
Aug 30, 2017
Merged

[REPL] Allow loading latest build from specified branch#1356
Daniel15 merged 2 commits intobabel:masterfrom
Daniel15:repl-branch

Conversation

@Daniel15
Copy link
Copy Markdown
Member

@Daniel15 Daniel15 commented Aug 29, 2017

  • Allows specifying a branch name in place of the build number (eg. /repl/build/7.0/ on prod, /repl/#?build=7.0 on dev). When specified, the latest successful build from the given branch is used. The code assumes that a numeric value is a CircleCI build number, and anything else is a branch name.
  • Modernizes the code a bit by using async/await, now that we can do that (yay Babel). This makes the overall flow through the code much clearer than nested callbacks.

Test Plan:
Version number loads the given version:

Branch name loads the latest build from the branch:

Build number loads that build (eg. for a pull request):

Invalid branch name throws a relevant error:

No parameters loads latest stable:

Closes #1352

@babel-bot
Copy link
Copy Markdown
Contributor

babel-bot commented Aug 29, 2017

Deploy preview ready!

Built with commit 13d095a

https://deploy-preview-1356--babel.netlify.com

@hzoo
Copy link
Copy Markdown
Member

hzoo commented Aug 29, 2017

Amazing work @Daniel15!! (also can we use fetch then if we are using promise)?

cc @gsathya

  • Also should we put a tooltip, docs, etc somewhere to explain the new url structure? can do in a separate pr

@gsathya
Copy link
Copy Markdown
Member

gsathya commented Aug 29, 2017

💯 🎉 That was fast! Thanks @Daniel15!

@hzoo
Copy link
Copy Markdown
Member

hzoo commented Aug 29, 2017

Also, what do we link about adding a link to the branch/commit/pr on the page somewhere @bvaughn

I'l just make a new issue #1359

@hzoo hzoo mentioned this pull request Aug 29, 2017
@Daniel15
Copy link
Copy Markdown
Member Author

can we use fetch then if we are using promise

Sure! I think I'll need to load a polyfill as some major browsers don't support it (IE 11, Android <= 4.4.4, iOS Safari <= 10.2, Safari <= 10), but there's some nice small polyfills. On the Yarn site we use unfetch, which is tiny.

I'll change it before merging.

@existentialism
Copy link
Copy Markdown
Member

@Daniel15 sounds good!

@bvaughn
Copy link
Copy Markdown
Contributor

bvaughn commented Aug 29, 2017

Amazing work @Daniel15!! (also can we use fetch then if we are using promise)?

Agreed! Great work! 😁

I've been wanting to use fetch for a while now but haven't suggested it because of the required IE/Edge polyfills. That being said, the polyfills are so tiny, as @Daniel15 says, we should just do it.

Also, what do we link about adding a link to the branch/commit/pr on the page somewhere

Maybe I could add this once we're comfortable removing the "classic" link?

Comment thread js/repl/Repl.js
_setupBabel() {
loadBabel(this.state.babel, this._workerApi, babelState => {
this.setState(babelState);
async _setupBabel() {
Copy link
Copy Markdown
Contributor

@bvaughn bvaughn Aug 29, 2017

Choose a reason for hiding this comment

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

async / await will require us to use the runtime polyfill, right? Which is pretty big.

I like the syntax much better than chaining promises but have been holding off for this reason.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah... I haven't measured the weight of it yet. I could switch it back to regular promises but async/await is so nice 😢 Particularly for code like this where you need to conditionally await something:

      if (!isBuildNumeric) {
        // Build in URL is *not* numeric, assume it's a branch name
        // Get the latest build number for this branch
        build = await loadLatestBuildNumberForBranch(
          config.circleciRepo,
          build
        );
      }
      const url = await loadBuildArtifacts(config.circleciRepo, build, doLoad);

I figured the developer experience is worth the overhead, but I'll need to see exactly how large the overhead is.

There's an alternative transform that transforms them into generators instead, which is lighter but browser support isn't as good (https://caniuse.com/#feat=es6-generators).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree- the syntax is super nice. But babel-polyfill.min is 104.56 kB. That's why the new REPL only lazily loads it if the user selects "Evaluate"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not dead set against changing this. It just seemed...questionable.

If we decide it's okay to add that weight, I'll happily replace the other promises with async/await.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not loading babel-polyfill, I'm only loading the Regenerator runtime. It looks like that's 6.8 KB gzipped unminified (https://unpkg.com/regenerator-runtime@0.11.0/runtime.js) but I haven't checked the minified size yet.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Daniel15 + babel-preset-env ~1mb 🤕

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh! I didn't realize you could load just the regenerator runtime separately.

That's great then! Let's definitely do that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PS. With regard to babel-preset-env size, we're lazy-loading everything that we can in the new REPL. What we can't lazy-load (like Babel standalone and React) we're at least loading from a CDN.

Any ideas to improve this are welcome though!

Copy link
Copy Markdown
Member

@hzoo hzoo Aug 29, 2017

Choose a reason for hiding this comment

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

^ btw we should be using useBuiltIns if we are using env so it would remove polyfills that aren't used. also loose mode if you want a smaller bundle

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Using built-ins is a configurable option ^

@Daniel15
Copy link
Copy Markdown
Member Author

I'm happy just using regular XMLHttpRequest, but a lot of people like fetch, so it's fine to add 👍

@yavorsky
Copy link
Copy Markdown
Member

Nice!!!

Also, what do we link about adding a link to the branch/commit/pr on the page somewhere

In the future maybe add the toggle to show commit history with the ability to select one from the list to reload required Babel's build according to the selected commit? Also, we can switch the list's mode to show available versions/branches etc.

Smth like:
screen shot 2017-08-29 at 20 15 21

@bvaughn
Copy link
Copy Markdown
Contributor

bvaughn commented Aug 29, 2017

In the future maybe add the toggle to show commit history with the ability to select one from the list to reload required Babel's build according to the selected commit? Also, we can switch the list's mode to show available versions/branches etc.

I wonder if that's overkill for the REPL? 😅 Maybe you could open an issue for discussion on that?

@yavorsky
Copy link
Copy Markdown
Member

yavorsky commented Aug 29, 2017

I wonder if that's overkill for the REPL?

Maybe, but it's the way to ignore url query manipulation + allow to easy switching between versions to detect when something stopped worked :)

I already have some work on this, but currently switched to implementing a normal standalone version of babel-preset-env 2.0.

@Daniel15
Copy link
Copy Markdown
Member Author

currently switched to implementing a normal version standalone of babel-preset-env 2.0.

I assume you could reuse most of babel/babel-standalone#90 for this? I made the babel-standalone Gulpfile reusable for this use case - You can use the same Gulp tasks as part of the -env build. See babel/minify#679 for example.

@yavorsky
Copy link
Copy Markdown
Member

yavorsky commented Aug 29, 2017

@Daniel15 Yes, the idea was to get rid of the babel-preset-env-standalone and use just original repo. There we wrap buildPreset and pass all plugins/presets to the callback. Consequently, we got the copy-paste from the original buildPreset and we need to update it every time we updating index.js from the babel-preset-env. So, we need an onCompile implementation in the original repo which works well with use-built-ins plugins which detect polyfills dynamically. I've almost finished and going to open PR to preset-env today.

@bvaughn

PS. With regard to babel-preset-env size, we're lazy-loading everything that we can in the new REPL. What we can't lazy-load (like Babel standalone and React) we're at least loading from a CDN.
Any ideas to improve this are welcome though!

The problem is simple and solvable. babel-preset-env uses same plugins as preset-es2015/es2016/es2017 (which actually included in Babel-standalone). So we have at least 2 versions of each plugin (except react). We need to let babel-preset-env know use plugins from the babel-standalone (on the build stage ? :/).

@Daniel15
Copy link
Copy Markdown
Member Author

I've almost finished and going to open PR to preset-env today.

@yavorsky, awesome! Feel free to @-mention me on the PR

We need to let babel-preset-env know that it need use plugins from the babel-standalone

babel-standalone already exports all the plugins, so that should be doable right?

@yavorsky
Copy link
Copy Markdown
Member

yavorsky commented Aug 29, 2017

babel-standalone already exports all the plugins, so that should be doable right?

So, babel-preset-env should import them 😄 . Or we need to come back to standalone-wrapper which can everything we want :)

@Daniel15
Copy link
Copy Markdown
Member Author

Daniel15 commented Aug 30, 2017

To recap, here's some examples of valid URLs:

  • /repl/ - Latest stable release
  • /repl/version/6/ - Latest version matching semver pattern 6 (currently 6.26.0). This can be any semver pattern (eg. /repl/version/~6.25) as it's resolved by Unpkg.
  • /repl/build/4713/ - Build #4713 from CircleCI (https://circleci.com/gh/babel/babel/4713). We mainly use this to load pull requests
  • /repl/build/7.0/ - Latest CircleCI build on the 7.0 branch

/version/xxxx/ loads published builds from npm (via Unpkg), while /build/xxxx/ loads CI builds from CircleCI.

@Daniel15 Daniel15 merged commit de7c5b5 into babel:master Aug 30, 2017
@Daniel15 Daniel15 deleted the repl-branch branch August 30, 2017 05:21
hzoo pushed a commit that referenced this pull request Sep 5, 2017
* [REPL] Allow loading latest build from specified branch

* Use fetch
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.

7 participants