[RFC] Replace react-tools

[zpao]

This is still a WIP but I wanted to get some feedback before I go too far. The primary goal is to replace the react-tools npm module (see #73 for some more info).

I did move all of the visitors because I thought this (mostly) makes more sense.

• add JSX transform
• expose simple API for node (combo of current react-tools API + babel)
  • transform(code, options)
  • transformFile(file, options, callback), transformFileSync(file, options)
• keep old API somewhere (require('jstransform/advanced') perhaps?)
• add the cli (just going to use commoner and mostly the same options from jsx)
• create a browser package akin JSXTransformer (though this can be skipped and we can just ship this from React for a while)
• update licensing (pulling in our own BSD stuff)
• update docs/readme

[zertosh]

This is great!

expose simple API for node (combo of current react-tools API + babel)

A good way to include/exclude transforms would be awesome. I know there is getVisitorsBySet and getAllVisitors. getAllVisitors works for excluding, but there is no include-only. There is also no way to get a list of the available transforms by name. Sure, you can fs.readdir the visitors directory but that's gross.

I've been experimenting with creating custom app builds for different browsers. The next stable version of Chrome will have class support on by default and it already supports template strings. Having a nice API for including/excluding visitors will go a long way towards making something like this less painful. And as the environment feature support improves, it makes sense to not want to apply every harmony transform.

This may not be the issue to bring this up in, but since this change will likely merit a bump in major version, I'll bring it up anyway. It's cool that ASTs are cached. That's only really useful if everything is using the same physical jstransform (I won't get into npm's deduping mess). The change proposed here should go a long way towards eliminating multiple copies of jstransform from the dep tree. But the caching system is still an all or nothing option. It'd be nice to have a way to expire it. Or maybe even just use something like lru-cache. Anything would be nice.

[zpao]

Totally agree on a better way to expose the visitors. I just added src/visitors/index which is the first step along the way. I think we probably want to expose a bit more (namely the keys in the object we already have).

I didn't expose flow there yet - AFAIK we still need to run that in a standalone transform pass. We can do that automatically in the simple API and just leave that as is with a note for the advanced API).

As for caching... it probably makes sense to make it possible to clear the cache. I'm not working on this full-time so I'm open to any ideas (and contributions 😉). My main goal is just to kill react-tools and anything else we can get in here is a bonus.

[appsforartists]

@zertosh

Are you sure you want the added machinery of serving different files to different browsers? Nevermind the pain and brittleness of sniffing; new implementations and polyfills often diverge in subtle ways that can break your app. I'd much rather use jstransform until I know the browsers I support have implemented Harmony correctly than try to be clever about shipping different builds to different browsers. Nobody likes the fire drill that happens when you realize Chrome shipped a new build this week that implemented an incompatible version of a feature you've been polyfilling.

[zertosh]

I'm not working on this full-time so I'm open to any ideas (and contributions 😉)

@zpao heh will do.

I'd much rather use jstransform until I know the browsers I support have implemented Harmony correctly than try to be clever about shipping different builds to different browsers.

I'm taking the long view on ECMAScript. If we really are transitioning to a world where ECMAScript is on a rapid-release schedule then js engines will never be "done". As old features become ubiquitous, new features get standardized, and transform tools add/keep for support them, you'll want to be selective. That's why 6to5 became babel 😄.

I think it's fair to say that there are certain parts of features that you can count on working with a high degree of confidence as long as you don't do Weird Shit. Take template strings: I feel good about using basic interpolation, but counting on the immutability of the raw parts in tagged template strings - ehhh.

The web isn't new to this. We live it everyday with CSS and vendor prefixes with weird behavior.

[appsforartists]

@zertosh my experience writing a polyfill for HTML5 forms, then having my app break in Chrome when they shipped a partially-broken implementation is what motivates my comments. 😃

[zpao]

Alright, I haven't tested much (except that the CLI technically runs).

I added a --react options to the CLI, which is what enables the JSX & displayName transforms. This is the big departure from the jsx command (which obviously transforms JSX by default).

I'm considering punting on the browser package thing. We can continue to build JSXTransformer in React for now. It would be good to come back to it soon though (but I think we want to think more about how we load the scripts... type="text/jsx" doesn't really make much sense in the general JS transform case).

For now I've kept the old API in place. This isn't essential and I've gone back and forth on this. It probably makes sense to get rid of them entirely.

Apart from that last bit, I think this is mostly "done". It'd be great to get a bit of feedback before I take it across the finish line.

cc @DmitrySoshnikov @amasad @jeffmo

[appsforartists]

Adding @petehunt too, since he maintains node-jsx and jsx-loader.

[zpao]

Ok, I think apart from writing docs, I'm done here. I went back and left the visitors where they were. I also left the current API in place - it was easier. The new API is available via require('jstransform/simple'). If that ends up being the wrong call, we're a major version bump away.

Any more feedback before I ship this? Anybody want to give it a try?

[zertosh]

@zpao I can take it for a spin tomorrow... in production

[zertosh]

Shouldn't id be passed in as filename on options so that the source maps reflect it?

[zpao]

That's a good question… probably but I've never actually used the sourcemaps option so I'm not sure. I'll check it out.

[zertosh]

Once I fixed the visitor names locally, it all ran smoothly :) ** For my uses, I don't use the CLI or modules.

[zpao]

Awesome, thanks for testing @zertosh!

[appsforartists]

Any idea when this will be published to npm?

[zpao]

soooooooon (today or tomorrow)

[appsforartists]

👏

We'll have to have @petehunt on deck to bump jsx-loader and node-jsx. 😃

[appsforartists]

(or, if he prefers, transfer them to github.com/facebook so they can be released in conjunction with jstransform).

[zpao]

I'll talk to him (can't just transfer because lawyers...)

[petehunt]

reporting for duty

[zpao]

Will write the readme separately, but getting this in now since we're all synced internally.

[appsforartists]

PRs to upgrade downstream:

• Upgrade to jstransform (from react-tools) petehunt/node-jsx#40
• Upgrade to jstransform (from react-tools) petehunt/jsx-loader#54