-
Notifications
You must be signed in to change notification settings - Fork 66
Conversation
This is great!
A good way to include/exclude transforms would be awesome. I know there is I've been experimenting with creating custom app builds for different browsers. The next stable version of Chrome will have 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 |
Totally agree on a better way to expose the visitors. I just added 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. |
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. |
@zpao heh will do.
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. |
@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. 😃 |
a4da6df
to
c992bbb
Compare
Alright, I haven't tested much (except that the CLI technically runs). I added a 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... 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. |
Adding @petehunt too, since he maintains node-jsx and jsx-loader. |
b83e9c2
to
3635d65
Compare
b3b2a6f
to
9723fd3
Compare
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 Any more feedback before I ship this? Anybody want to give it a try? |
@zpao I can take it for a spin tomorrow... in production |
// This is where JSX, ES6, etc. desugaring happens. | ||
// We don't do any pre-processing of options so that the command line and the | ||
// JS API both expose the same set of options. | ||
var result = transform(source, this.options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't id
be passed in as filename
on options so that the source maps reflect it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Once I fixed the visitor names locally, it all ran smoothly :) ** For my uses, I don't use the CLI or modules. |
Awesome, thanks for testing @zertosh! |
14a8b0b
to
69da664
Compare
Any idea when this will be published to npm? |
soooooooon (today or tomorrow) |
👏 We'll have to have @petehunt on deck to bump jsx-loader and node-jsx. 😃 |
(or, if he prefers, transfer them to github.com/facebook so they can be released in conjunction with jstransform). |
I'll talk to him (can't just transfer because lawyers...) |
This is working towards a simple API. This is taken from how we end up using jstransform in React and internally. We build up these lists and expose some APIs to get them. We can tweak this (it might make sense to expose the keys of tranformVisitors).
This mostly behaves the same way react-tools did. One big difference is that the transform function exported does not have a simple string return value. This is more like transformWithDetails. This is inline with how babel does it, and I like it. We already have the information, we might as well expose it and leave it to the consumer to get what it wants off.
Trailing commas simply strips the trailing commas from objects and arrays. This is allowed in the ES5 spec but not all environments support it. Undefined to void 0 is self explanatory. It's helpful for situations where undefined may be overwritten. Some research shows (void 0) may be more performant that undefined.
69da664
to
8482296
Compare
reporting for duty |
Will write the readme separately, but getting this in now since we're all synced internally. |
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.
transform(code, options)
transformFile(file, options, callback)
,transformFileSync(file, options)
require('jstransform/advanced')
perhaps?)jsx
)