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

Add AST panel to repl #1421

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@danez
Copy link
Member

commented Oct 15, 2017

This adds tabs to switch between babel output and AST.

I fixed also an issue, that an empty source was still compiled. I changed it to skip compilation if source is empty.

I had to change how the codemirror is rendered because of performance issues. The AST output can be quite large and it took 5 seconds to switch tabs. With the change from textarea to div and positioning the div absolute it is instant. (I found this behaviour on astexplorer)

There is one bug still, that I haven't figured out yet. In mobile when the options are uncollapsed and then get collapsed the editor is not using the complete height.

Design wise I am open to suggestions, I'm really not good with making design choices :)

bildschirmfoto 2017-10-15 um 12 36 28

@danez danez force-pushed the ast branch from 1f9502b to 623c75a Oct 15, 2017

@babel babel deleted a comment from babel-bot Oct 15, 2017

@xtuc

This comment has been minimized.

Copy link
Member

commented Oct 15, 2017

Really cool, nice job.

It seems that it added another scrolling bar because of the new header which is taking some space.

@danez

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2017

In which browser?

@xtuc

This comment has been minimized.

Copy link
Member

commented Oct 15, 2017

I'm running Chrome version 58.0.3029.110 (64-bit).

This is what I can see:
screenshot from 2017-10-15 13-34-03
screenshot from 2017-10-15 13-34-14

@danez

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2017

Okay found it and fixed it, thanks @xtuc

Also added the time the transformation took to the toolbar now.

@xtuc

This comment has been minimized.

Copy link
Member

commented Oct 15, 2017

Fixed on my side, thanks @danez

@babel babel deleted a comment from babel-bot Oct 15, 2017

@existentialism
Copy link
Member

left a comment

LGTM.

Fine with the design, we can put an examples dropdown in the space above the left side editor.

@xtuc

xtuc approved these changes Oct 15, 2017

@danez danez requested a review from bvaughn Oct 15, 2017

@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2017

Where's the Netlify preview build comment? 😄

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

@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2017

I like having the AST output handy, but I don't like losing a line of vertical space fro the tab bar.

I wonder if we could add this as a togglable option in the sidebar's "Settings" panel somehow? Maybe something like...

screen shot 2017-10-15 at 2 54 47 pm

It's probably not as obvious / discoverable as the tab bar but:

  • I don't think it needs to be super visible b'c I don't think it's a common use-case.
  • I think it's worth the tradeoff to avoid using valuable vertical space.
@hzoo

This comment has been minimized.

Copy link
Member

commented Oct 15, 2017

@bvaughn has a good point about the vertical space, and I guess it would mostly make sense to reuse the menu bar we already have on the left in some way?

And yeah adding some basic examples would be good, we should probably do that with the ones in learn es2015 at least (simple to do)

@existentialism

This comment has been minimized.

Copy link
Member

commented Oct 15, 2017

Yea... I'm fine with it in the sidebar. Guess we can solve the examples stuff later if we ever even get to it.

@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2017

Alternately we could maybe let people show/hide all 3 for more horizontal space too? Maybe this is inventing a feature that no one wants though 😁

screen shot 2017-10-15 at 2 59 57 pm

Guess we can solve the examples stuff later if we ever even get to it.

What "examples stuff"? 😄

@hzoo

This comment has been minimized.

Copy link
Member

commented Oct 15, 2017

regarding examples: wanted a way to pre-fill in some input code to test out syntax etc for new users #1263, ideally from real code though

cc @fkling ideally we would just use astexplorer somehow fkling/astexplorer#274 😸

@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2017

regarding examples: wanted a way to pre-fill in some input code to test out syntax etc for new users #1263, ideally from real code though

I may be misunderstanding you, but isn't this already doable just via the query params?

@hzoo

This comment has been minimized.

Copy link
Member

commented Oct 15, 2017

@bvaughn Oh it is, just need a UI for it / the data/code snippets themselves

oh and a purpose of doing this is that once we finally move babylon back into this repo (this @danez!) then we can test parser PRs using the AST output as well

and maybe interesting to try https://github.com/pretty-format/pretty-format-ast

@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2017

@bvaughn Oh it is, just need a UI for it / the data/code snippets themselves

Wouldn't the UI just be links (eg "Try this in the REPL") mixed throughout the non-JS portion of the site?

Or were you imagining some menu within the REPL to view pre-installed "recipes"?

@hzoo

This comment has been minimized.

Copy link
Member

commented Oct 15, 2017

Imagining some menu within the REPL to view pre-installed "recipes"?

If that would be useful yes, but you are right it's already in those places as well true

@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2017

I kind of like keeping the REPL simple- kind of like a browser-based IDE. I think an external recipes section (eg somewhere else on the website) with links to the REPL for common code patterns could be nice though.

@hzoo

This comment has been minimized.

Copy link
Member

commented Nov 2, 2017

We should definetely land this in some form now that babylon is in the monorepo, and it would be nice to have it remove loc or other ways to filter.

Although now that I think about it we could always just use this url to test haha, or land fkling/astexplorer#274

@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2017

I'm not 100% sold on the need for AST output here. Seems like it overlaps with AST Explorer, which is focused solely on AST exploration (and as such is much better IMO).

That being said, I still have the same concern I raised previously about the loss of vertical space with this change.

If you like either of the sidebar options I mentioned in previous comments, I can probably implement one of them on the flight to London tomorrow. Just let me know which you prefer.

@hzoo

This comment has been minimized.

Copy link
Member

commented Nov 3, 2017

Yeah, astexplorer does it much better for sure: has highlighting, filtering of properties, etc. We just want to be able to see the ast output per PR, but that can be alternatively dealt with by just running this PR's link: https://deploy-preview-1421--babel.netlify.com/repl/build/master or getting astexplorer itself to read any babel-standalone version just like babel repl is doing?

@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2017

but that can be alternatively dealt with by just running this PR's link

Ha! I understand what you meant by that comment now 😆 Yeah, assuming Netlify never cleans up "preview" URLs, you're right.

or getting astexplorer itself to read any babel-standalone version just like babel repl is doing?

Yup! That could also be nice! 😁

@danez

This comment has been minimized.

Copy link
Member Author

commented Nov 3, 2017

For me it is an essential feature missing from our repl, especially when working on pull requests to see what the AST looks like is something I do all the time for babylon. Right now I create dummy tests and change them, run tests, change test, run tests,... just to see the AST, which is not a nice workflow. Also reproducing bugs with the latest master is something i do regularly and ASTexplorer always being outdated doesn't help with that.

I personally don't care where the option for the AST lives even if its a tab row, as screens are big enough imho and nobody is probably seriously using the repl on a phone. Also if we want to integrate other stuff the top bar could be useful.
On the other hand if we want to have options to disable output of locations for example then they might live in the sidebar, but splitting options is also not good I guess, so probably everything should be moved to the sidebar.
I also like the idea of being able to see the output and the AST at the same time.

I guess the main points of the repl is to let people see what babel is doing (where examples could come handy), let people reproduce bugs in an easy way (source code as well as AST) and for maintainers to investigate bugs.

When I have time I will remove the tabbar and move the AST option to the sidebar and add an option to remove locations.

@hzoo hzoo referenced this pull request Nov 9, 2017

Closed

tc39-proposal-pattern-matching #6761

3 of 3 tasks complete
Daniel Tschinder
Add option to show AST output
Also fixes an issue when the textarea is cleared.

@danez danez force-pushed the ast branch from 71d6115 to e4c4154 Dec 12, 2017

@danez

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2017

I changed it now to be a separate panel instead of tabs.

@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2017

I like this change, Daniel!

@bvaughn
Copy link
Contributor

left a comment

This looks great to me. Thanks for following up on this!

}
const container = this._container;
container.removeChild(container.children[0]);
this._codeMirror = null;

This comment has been minimized.

Copy link
@bvaughn

bvaughn Dec 13, 2017

Contributor

I guess we didn't actually need to call toTextArea() ? 😄

This comment has been minimized.

Copy link
@danez

danez Dec 13, 2017

Author Member

As the source is not a textarea anymore, it doesn't make sense to convert back to textarea. I stole this from somewhere, but cannot recall from where.

@xtuc

xtuc approved these changes Dec 13, 2017

Copy link
Member

left a comment

That's awesome! Thanks Danez!

delete transformed.ast.tokens;
}

ast = JSON.stringify(transformed.ast, null, 2);

This comment has been minimized.

Copy link
@xtuc

xtuc Dec 13, 2017

Member

I'm just wondering, the AST shows the emitted code, wouldn't it more practical to show the input?

For example JSX: REPL. I would expect to see the JSX* instead of the React.createElement in the AST.

This comment has been minimized.

Copy link
@danez

danez Dec 13, 2017

Author Member

Good point, you can of course by hand disable all presets, but that might be forgotten.

So I see two things we can do here:

  • Either ensure that no presets and plugins are supplied to babel when outputing the AST.
  • Have a separate compiler that uses babylon directly.

The first one is probably easier. And in any case all other options (presets, env) should be disabled in the UI when selecting AST.

This comment has been minimized.

Copy link
@xtuc

xtuc Dec 13, 2017

Member

I would take the second option.

You have two possible outputs:

  • A Babel transform (as currently implemented)
  • An AST, Babylon could directly parse it using a few Babylon plugin from the plugins/presets.

(Babylon can be loaded from bundle.run, we don't even need a standalone version)

@hzoo

This comment has been minimized.

Copy link
Member

commented Feb 23, 2018

You're free to land this on master and old-site if you want

@xtuc
Copy link
Member

left a comment

Currently it's showing the transformed AST, I think it would be better to show before any transformations were applied.

It's confusing for users because they wouldn't see JSX* nodes but rather the CallExpression version.

@danez danez closed this Feb 9, 2019

@danez danez referenced this pull request Feb 9, 2019

Open

AST Explorer #1933

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.