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

React rewrite of Babel REPL #1297

Merged
merged 33 commits into from
Aug 15, 2017
Merged

React rewrite of Babel REPL #1297

merged 33 commits into from
Aug 15, 2017

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Aug 7, 2017

Relates to #1287; cc @hzoo, @Daniel15, @existentialism, @CompuIves

Preview available at bvaughn.github.io/babel-repl

High-level features

  • Comlpetely rewritten with React for easier future maintanence.
  • Fully Flow-typed.
  • Large plugins (eg Babili, Prettier) load on-demand, only if enabled.
    • Loading doesn't block compilation; once a plugin loads, code will be auto-recompiled.
    • This framework can be extended for third party plugins (loaded directly from NPM).
  • Large libraries like Codemirror and Babel are loaded from a CDN to better share with other sites (eg prettier.io).
  • Mobile-friendly layout.

Structure

.
├── _site
│   ├── repl  # current REPL source
│   └── repl2 # new REPL build output
└── js
    └── repl  # new REPL source

Open questions

  • Are we okay with using create-react-app for the React portions of the site or should I trim down the Webpack+Babel configuration.

Follow-up items in the "New REPL" milestone

@DanBuild
Copy link
Collaborator

DanBuild commented Aug 7, 2017

Deploy preview ready!

Built with commit 4951d6f

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

@hzoo hzoo added the repl label Aug 7, 2017
@hzoo
Copy link
Member

hzoo commented Aug 7, 2017

another feature we should port over later is handling console.log and friends

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 7, 2017

"console.log and friends"? What do you mean? 😄

@hzoo
Copy link
Member

hzoo commented Aug 7, 2017

Sorry was trying to have some fun, we have to capture the console and re-output it

https://github.com/babel/babel.github.io/blob/64c82a57d7e6552d8992ae54572ef82a7f97c426/scripts/repl.js#L593-L641

otherwise try doing a console.log in your repl and I don't think it would print anything

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 7, 2017

It's not obvious to me at a glance why we do this?

Copy link
Member

@Daniel15 Daniel15 left a comment

Choose a reason for hiding this comment

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

This looks pretty solid to me. I left a few initial comments inline. They're all very small, the actual components themselves look pretty solid to me.

It looks like it's not integrated into the Jekyll build yet, so unfortunately we can't see a live preview yet :(


npm-debug.log*
yarn-debug.log*
yarn-error.log*
Copy link
Member

@Daniel15 Daniel15 Aug 7, 2017

Choose a reason for hiding this comment

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

Can we merge this into the root .gitignore? I suspect that things we ignore in the REPL will also be useful to ignore for the rest of the site.

"name": "babel-repl",
"version": "0.1.0",
"private": true,
"homepage": "https://bvaughn.github.io/babel-repl",
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Remove

@@ -0,0 +1,52 @@
<!doctype html>
Copy link
Member

Choose a reason for hiding this comment

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

I think that ideally this should use the Babel site styling rather than being a totally separate HTML page. We probably want the Babel top bar at least, for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed! This is just detritus from my demo site. I haven't yet looked into how to share HTML with the main babel site. 😄

Copy link
Member

Choose a reason for hiding this comment

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

@bvaughn - Should be pretty straightforward. You just need to make a HTML file with "YAML front matter" so that Jekyll processes it. See how the old REPL does it: https://github.com/babel/babel.github.io/blob/master/repl.html

Copy link
Contributor Author

@bvaughn bvaughn Aug 7, 2017

Choose a reason for hiding this comment

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

Interesting. So the only tricky part is making it work nicely with dev workflow. (Adding front matter would break the yarn dev workflow for CRA.) Maybe this is a good enough reason to bail on CRA, although I like the idea of being able to test the REPL without running the whole (slow) Jekyll build. It's painful.

Copy link
Member

Choose a reason for hiding this comment

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

In theory, the JS can be rebuilt entirely separately from the Jekyll build, if you can configure it to build it directly into the _site directory. That way, you'd only care about the Jekyll build if you touch the HTML. I wonder if Jekyll would get angry at us for touching the _site files directly though. Could try it and see 😛

Yeah, Jekyll is slow... Maybe we should transition the site to something like Hugo, it's a lot faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory, the JS can be rebuilt entirely separately from the Jekyll build, if you can configure it to build it directly into the _site directory.

That's what the PR currently does (via yarn build) although nothing hooks into the make build process to run it.

Yeah, Jekyll is slow...

I've been waiting for npm start to run for about 5 minutes and I'm stuck at "Generating..." 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Wow, it shouldn't take that long... I think it normally takes ~5-10 seconds for me (which is slow, but bearable). I wonder if it's hung on something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems likely, although I rm -rf ./_site && npm start and it still seems to run to that point and then stick.

Copy link
Contributor Author

@bvaughn bvaughn Aug 8, 2017

Choose a reason for hiding this comment

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

Running bundle exec jekyll serve directly (with the --verbose flag) shows it getting hung after:

   GitHub Metadata: Initializing...
        Generating: Jekyll::GitHubMetadata::GHPMetadataGenerator finished in 0.001932 seconds.
        Generating: JekyllOptionalFrontMatter::Generator finished in 0.091768 seconds.
        Generating: JekyllReadmeIndex::Generator finished in 24.729063 seconds.

This is true on master as well. Hm. I wonder what broke. This worked for me at one point.

Edit 1

A fresh clone also fails, but in a different way:

git clone git@github.com:babel/babel.github.io.git ./babel-new
cd babel-new
bundle install
npm i
npm start

Fails with:

> babel.github.io@1.0.0 start /Users/bvaughn/Documents/git/babel-new
> make serve

bundle check || bundle install; \
  bundle exec jekyll serve
The Gemfile's dependencies are satisfied
Configuration file: /Users/bvaughn/Documents/git/babel-new/_config.yml
Configuration file: /Users/bvaughn/Documents/git/babel-new/_config.yml
            Source: /Users/bvaughn/Documents/git/babel-new
       Destination: /Users/bvaughn/Documents/git/babel-new/_site
 Incremental build: disabled. Enable with --incremental
      Generating...
  Liquid Exception: Could not locate the included file 'readmes/babel-core.md' in any of ["/Users/bvaughn/Documents/git/babel-new/_includes"]. Ensure it exists in one of those directories and, if it is a symlink, does not point outside your site source. in docs/usage/api.md
jekyll 3.4.3 | Error:  Could not locate the included file 'readmes/babel-core.md' in any of ["/Users/bvaughn/Documents/git/babel-new/_includes"]. Ensure it exists in one of those directories and, if it is a symlink, does not point outside your site source.
make: *** [serve] Error 1
npm ERR! code ELIFECYCLE
npm ERR! errno 2
npm ERR! babel.github.io@1.0.0 start: `make serve`
npm ERR! Exit status 2
npm ERR!
npm ERR! Failed at the babel.github.io@1.0.0 start script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/bvaughn/.npm/_logs/2017-08-08T00_07_53_930Z-debug.log

Edit 2

As far as the fresh checkout goes- looks like I needed to run npm run build before npm start. Didn't see that in the docs. Still having trouble with this checkout though. Hm.

Edit 3

Checking out my branch in the new repo and yarn building this project before starting Jekyll up again caused it to hang also. I'm not familiar with Jekyll so I don't know how fragile it's supposed to be. Seems to be pretty easy to hang it though. Anyone with more Jekyll experience willing to lend a hand?

I'm not particularly passionate about learning how Jekyll works to connect these dots. I'd rather spend energy adding more features to the REPL. 😁 I think I'd be more effective at that.

Copy link
Member

Choose a reason for hiding this comment

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

I'l just make a make bootstrap like babel's

@@ -0,0 +1,15 @@
{
"short_name": "React App",
"name": "Create React App Sample",
Copy link
Member

Choose a reason for hiding this comment

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

Not quite. 😛

Do we need this manifest? What does it do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. Just create-react-app boilerplate. 😛

@@ -0,0 +1,3 @@
import Repl from './repl/Repl.js';

export default Repl;
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of just exporting Repl? Could we just use repl/Repl.js directly?

_presetsToArray(state: State = this.state): Array<string> {
const { presets } = state;

return Object.keys(presets)
Copy link
Member

Choose a reason for hiding this comment

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

If this is being used as a map, could we use a Map rather than an object? Given the latest React beta requires Map to be polyfilled, we can likely rely on it existing?

const { code } = this.state;

this._checkForUnloadedPlugins();
this._updateCode(code);
Copy link
Member

Choose a reason for hiding this comment

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

Avoid the unnecessary object spread:

this._updateCode(this.state.code)

}, {});

const styles = {
codeMirrorPanel: css({
Copy link
Member

Choose a reason for hiding this comment

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

What's the advantage of all this CSS-in-JS stuff?

presetState,
toggleSetting
} = this.props;
console.log('<ReplOptions> lineWrap:', lineWrap, typeof lineWrap);
Copy link
Member

Choose a reason for hiding this comment

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

Remove?

}).code;

if (config.prettify && window.prettier !== undefined) {
// TODO Don't re-parse; just pass Prettier the AST we already have.
Copy link
Member

Choose a reason for hiding this comment

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

👍

@Daniel15
Copy link
Member

Daniel15 commented Aug 7, 2017

This is a super minor thing, but I think js/repl/src/repl/ is a bit silly since repl is repeated. In keeping with the existing layout of the site files, I'd suggest using css/repl/ for the CSS and scripts/repl/ for the JavaScript (scripts/repl.js and scripts/7.js is the current REPL).

Let's make this accessible at /replv2/ or /repl_new/ or something while we iterate. 👍

Fantastic work @bvaughn!

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 7, 2017

Thanks @Daniel15!

Agreed that repl/src/repl is a bit silly. It didn't feel appropriate to put it under js/ (since we may want to put other things there in the future?) I can certainly remove that nested repl folder though. There's no benefit in it.

I'd suggest using css/repl/ for the CSS and scripts/repl/ for the JavaScript

That would require replacing CRA with a new build pipeline. I don't think I have the time/interest in doing that (at least not until...maybe...the upcoming weekend).

Let's make this accessible at /replv2/ or /repl_new/ or something while we iterate. 👍

Running yarn build will sync the output to _site/repl2 (based on @hzoo's comment on the issue). As the TODO section states though, this still needs to be wired up to make build.

@hzoo
Copy link
Member

hzoo commented Aug 7, 2017

Oh the console handling is just so you can just console output in the site without opening devtools

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 7, 2017

Sorry for the clowny console.log statements I forgot to remove. Tried to tidy this up for PR this morning, last minute, between meetings and... 🤡 happens

{
label: 'es2015-loose',
package: 'babel-preset-es2015-loose',
version: '7'
Copy link
Member

@existentialism existentialism Aug 7, 2017

Choose a reason for hiding this comment

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

Just for my own understanding, these version numbers here currently don't do anything since we pull presets from babel-standalone, yea?

(Noticed this was 7 where the rest were 6)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. Yes. I wrote all of this intending to pull these from CDN too but then I realized standalone had them baked in. We could remove them.

Copy link
Member

Choose a reason for hiding this comment

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

Might still be useful re: babel-preset-env (at least until babel/babel-standalone#90 is merged), we can mark the presets that babel-standalone includes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. That can be part of the TODO for whoever does that (maybe me, I dunno).

Copy link
Member

Choose a reason for hiding this comment

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

Even after babel/babel-standalone#90 is merged, babel-preset-env will still be a separate package due to how large it is (as it needs all those compat lists to determine which plugins to execute). With all the other presets and plugins, they're small enough (compared to Babel itself) that there's not really an advantage of splitting them out into their own packages.

Copy link
Contributor Author

@bvaughn bvaughn Aug 7, 2017

Choose a reason for hiding this comment

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

Should be easy enough to accommodate this. We can add an isPreloaded attribute to PluginConfig and make the version attribute optional. I assumed this stuff will change as we add more JIT loaded presets and stuff. I tried to create an initial structure that was fairly flexible but I didn't stress it too much b'c the weekend was too short.

@hzoo
Copy link
Member

hzoo commented Aug 8, 2017

#1299 should make it easier! runs everything

btw if you want a more real-time chat, feel free to join us on slack at http://slack.babeljs.io/ - theres a #website room

@xtuc xtuc self-requested a review August 8, 2017 09:53
@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 8, 2017

Cool. Thanks Henry.

As I mentioned yesterday, I could use a hand getting the Jekyll setup finished. I'll try to knock out the remaining "TODO" items (other than that) over the next couple of days.

@hzoo
Copy link
Member

hzoo commented Aug 8, 2017

Are you still having the same issue in #1297 (comment)? not sure what the issue is right now

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 8, 2017

Yes. I have not looked into it further after mentioning in Slack last night that I'd prefer to spend my (limited) time finishing the non-Jekyll related TODO items.

@Daniel15
Copy link
Member

Daniel15 commented Aug 8, 2017

No problem @bvaughn. No pressure at all. If you're having difficulties getting it working with Jekyll and don't want to spend more time on it, just let us know when you've completed all the work that you want to do and someone else can take over the work to get it working with Jekyll. 👍

@Daniel15
Copy link
Member

Daniel15 commented Aug 8, 2017

For what it's worth, the Netlify build (which is what we use to deploy the production version of the site) is simply doing npm run build && jekyll build on a fresh clone and that's working fine.

@hzoo hzoo mentioned this pull request Aug 9, 2017
@ritz078
Copy link
Contributor

ritz078 commented Aug 10, 2017

@bvaughn Did to get prettier to work in safari ?

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 10, 2017

@ritz078 No, Safari has a complaint with the Prettier bundle syntax. I didn't look into it.

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 10, 2017

@Daniel15 Thanks! I've gotten side-tracked by ReactNative sync issues but I'll try to find time to wrap this up this weekend. (I likely won't spend much effort looking into Jekyll stuff though. I'll be happy to hand that off to someone with more experience.)

@xtuc
Copy link
Member

xtuc commented Aug 10, 2017

@ritz078 @bvaughn The issue with https://bundle.run/prettier@1.5.3 seems to come from Prettier. (issue in bundle.run)

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 10, 2017

Agreed! It looked like a Prettier+Safari issue which is why I didn't look into it. 😄

@Daniel15
Copy link
Member

Given the fact that Prettier is optional + Safari has very low market share, maybe it's not worth looking into that at the moment 😛 Broken in Firefox is more of an issue though. We can leave Prettier integration until later. If it's just due to some missing transforms, it might be more reliable to manually bundle Prettier using Webpack as then we can tweak the Webpack settings to get it just right.

@hzoo
Copy link
Member

hzoo commented Aug 10, 2017

This is just because prettier targets Node 4, and a lot of tools are compiling down to that instead of node 0.10 so it uses ES6 syntax (unlike Babel where we compile to ES5).

@ritz078
Copy link
Contributor

ritz078 commented Aug 10, 2017

For a temporary fix I am using https://github.com/beautify-web/js-beautify instead of prettier. Though it also supports jsx, its not as good as prettier for JSX.

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 13, 2017

I think this PR is ready for review 🎉

I've added a couple of follow-up items (explicitly listed in the PR summary) either because (a) I don't think we're quite ready to do them yet or (b) I don't think I'm the best person to do them.

I've updated the demo at bvaughn.github.io/babel-repl to match the latest PR changes.

Edit CI is failing b'c it doesn't like something in js/repl being copied over into _site/js/repl. That's kind of part of one of the follow-up items (hooking this into Jekyll correctly) but I guess we should at least address that part of it here. Open to suggestions. 😄

Copy link
Member

@Daniel15 Daniel15 left a comment

Choose a reason for hiding this comment

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

This looks pretty solid to me. I left a few comments inline but they're all very minor. I couldn't comment on favicon.ico as it's a binary file, ut we can probably avoid the favicon.ico file and just use the root favicon.

With regards to integrating it into Jekyll, would you like someone to fork your fork and send you a pull request for it (into your repl-2 branch) to get it into this pull request itself, or would you prefer to merge this and iterate?

One other question - Would it take much work to lazy load babel-standalone itself? In #1284 I changed Babel to be lazy loaded so that we could load a custom version (either from a CircleCI build, or a previous version from unpkg). I can work on integrating those changes into your version too, just wondering how difficult it'd be.

"prettier": "^1.5.3",
"react": "^16.0.0-beta.3",
"react-dom": "^16.0.0-beta.3",
"react-scripts": "1.0.10"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe merge this with the root package.json so we don't have to yarn install in two separate directories? I'm not sure what the benefit of splitting them is.

"eject": "react-scripts eject",
"flow": "flow",
"postbuild": "cp -r ./build ../../_site/repl2",
"prettier": "prettier --single-quote --write 'src/**/*.{js,jsx}'",
Copy link
Member

Choose a reason for hiding this comment

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

Likewise this would be good for the entire site.

homescreen on Android. See https://developers.google.com/web/fundamentals/engage-and-retain/web-app-manifest/
-->
<link rel="manifest" href="%PUBLIC_URL%/manifest.json">
<link rel="shortcut icon" href="%PUBLIC_URL%/favicon.ico">
Copy link
Member

@Daniel15 Daniel15 Aug 13, 2017

Choose a reason for hiding this comment

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

I guess we'll remove this %PUBLIC_URL% once the build of this page is moved into Jekyll as it handles the URLs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this HTML will be replaced by one with Jekyll frontmatter by whoever does that integration.

keyMap: 'sublime',
matchBrackets: true,
mode: 'text/jsx',
lineNumbers: true,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Alphabetize? This list of options is almost alphabetical, except for lineNumbers being in the wrong spot. 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops.

"eject": "react-scripts eject",
"flow": "flow",
"postbuild": "cp -r ./build ../../_site/repl2",
"prettier": "prettier --single-quote --write 'src/**/*.{js,jsx}'",
Copy link
Member

Choose a reason for hiding this comment

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

Likewise, this would be good to have for the entire site, not just for the REPL.

Copy link
Member

Choose a reason for hiding this comment

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

(I don't know why this comment appears twice)

});

const styles = {
loadingAnimation: css({
Copy link
Member

Choose a reason for hiding this comment

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

Are all these styles extracted into real CSS files on build?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're inserted by JavaScript using insertRule, why?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that's fine too. As long as it's not inline styles since they're not great for perf.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Glamor is really performant. I'm using it for the new React website too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Plus @threepointone works with us now and he's really helpful. 😁

Copy link
Member

@Daniel15 Daniel15 Aug 13, 2017

Choose a reason for hiding this comment

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

The reason that I was asking was that some other solutions need you to extract the CSS into a regular CSS file for best performance. See this benchmark from Emotion, for example: https://github.com/tkh44/emotion/blob/master/docs/benchmarks.md. I wasn't sure if Glamor has the same perf characteristics.

I'm using it for the new React website too.

Is the new React website using server rendering with CSS-in-JS? That's an interesting approach, I haven't seen anyone do that before 😛 Or does the site require JS just to render static pages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new React site is built with Gatsby, which does SSR, and uses the Glamor plug-in which is optimized for the server-rendering.

presetState: PluginStateMap,
runtimePolyfillConfig: PluginConfig,
runtimePolyfillState: PluginState,
toggleEnvPresetSetting: ToggleEnvPresetSetting,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe name all the callbacks with an "on" prefix? Up to you though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't feel strongly about this so 👍


type Props = {
className: string,
path: string
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call this component SvgPath since it's specifically for rendering a path?

Copy link
Contributor Author

@bvaughn bvaughn Aug 13, 2017

Choose a reason for hiding this comment

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

That's pretty strongly implied, no? It's a property of an element named Svg 😁 I'm not normally a fan for redundant prefixes in cases like this.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. I just thought it might get confusing.

// Just evaluate the most recently compiled code.
try {
// eslint-disable-next-line
eval(this.state.compiled);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this only happen if evaluation is enabled? I saw a config.evaluate check for the other eval call.

Copy link
Contributor Author

@bvaughn bvaughn Aug 13, 2017

Choose a reason for hiding this comment

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

We'll only hit these lines if "evaluate" is checked. (It toggles runtimePolyfillState.isEnabled. The comments a few lines up mention this.) These lines handle the edge-case where we've just checked "evaluate" but the polyfill hasn't yet been loaded.

Copy link
Member

Choose a reason for hiding this comment

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

Oops, thanks for the correction :)

margin: 0;
padding: 0;
font-family: sans-serif;
}
Copy link
Member

Choose a reason for hiding this comment

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

I guess this body styling can be removed once we merge the REPL into the site itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Correct! The only bit I'd want to keep (probably) would be

box-sizing: content-box

* Alpha-sort Codemirror props
* Added some inline comments
* Renamed toggle* props to on*Change
@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 13, 2017

This looks pretty solid to me. I left a few comments inline but they're all very minor. I couldn't comment on favicon.ico as it's a binary file, ut we can probably avoid the favicon.ico file and just use the root favicon.

Sure. This was just part of a standard create-react-app boilerplate. Once we use the Jekyll partial template thingy, the favicon would be the main Babel one anyway.

Thanks for the review! 😁

With regards to integrating it into Jekyll, would you like someone to fork your fork and send you a pull request for it (into your repl-2 branch) to get it into this pull request itself, or would you prefer to merge this and iterate?

Either is fine! 😄 Slight preference to merge and iterate~ but I'll defer to you.

One other question - Would it take much work to lazy load babel-standalone itself? In #1284 I changed Babel to be lazy loaded so that we could load a custom version (either from a CircleCI build, or a previous version from unpkg). I can work on integrating those changes into your version too, just wondering how difficult it'd be.

Not difficult. But probably okay to do as a follow-up item? (I don't mind doing it, just trying to prevent this PR from growing too much more.)

@Daniel15
Copy link
Member

But probably okay to do as a follow-up item?

Totally fine!

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 13, 2017

But probably okay to do as a follow-up item?

Totally fine!

We can turn the follow-up items (including this) into issues in the "new repl" milestone @hzoo created.

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 14, 2017

I've created tickets in the REPL milestone for all of the follow-up work.

Copy link
Member

@existentialism existentialism left a comment

Choose a reason for hiding this comment

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

Fantastic stuff @bvaughn!

I'm good with landing this and attacking the rest in follow up PRs. I have some bandwidth this week to tackle a few too!

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 14, 2017

Thanks @existentialism!

Copy link
Member

@xtuc xtuc left a comment

Choose a reason for hiding this comment

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

awesome, nice job @bvaughn ❤️

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 14, 2017

Thanks @xtuc!

I'm not sure if we should merge this yet or hold off for the Jekyll follow-up (#1309). Looks like CI is busted in master anyway so maybe it doesn't matter. I'll defer though since I'm still the noob here. 😄

@Daniel15
Copy link
Member

The only issue with merging it now is that nobody is actually going to see it, since the HTML file is not yet accessible on the site 😛 Maybe that's okay for now though.

Looks like CI is busted in master

CI is only broken due to a lint error in one of the new blog posts. The actual website build is not broken.

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 15, 2017

Okay! Here we go then! 🎉

@bvaughn bvaughn merged commit 9e24125 into babel:master Aug 15, 2017
@bvaughn bvaughn deleted the repl-2 branch August 15, 2017 00:32
@Daniel15
Copy link
Member

Thanks @bvaughn! Since you were having issues with Jekyll, I'll see if I can get that part working. 👍

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 15, 2017 via email

@Daniel15
Copy link
Member

I worked out why you were having problems with Jekyll. The js/repl/node_modules directory is not in Jekyll's list of excluded directories, so it was trying to include all ~20,000 files in that directory as part of the website build 😛

It took a while to track down, I only figured it out after watching exactly what the Ruby process was doing, and noticed that it was reading a large number of Markdown files from node_modules:

Anyways, I'll send a PR once I have something working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants