Skip to content

Add more useful ES6 transforms to jsx-internal. #636

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

Merged
merged 1 commit into from
Dec 6, 2013

Conversation

cpojer
Copy link
Contributor

@cpojer cpojer commented Dec 5, 2013

Comment is self-descriptive - it is high time we start using the good stuff.

I will follow up with removing 'arguments' from React and replacing it with rest-params.

@vjeux
Copy link
Contributor

vjeux commented Dec 5, 2013

Nice!

@benjamn
Copy link
Contributor

benjamn commented Dec 5, 2013

Awesome.

@jordwalke
Copy link
Contributor

Could you throw in the quasi-literals transform if it's not already in here?

@cpojer
Copy link
Contributor Author

cpojer commented Dec 5, 2013

Sure, I just didn't think we would need them right now (do we have a reason to use them atm?)

@jeffmo
Copy link
Contributor

jeffmo commented Dec 5, 2013

The changes here lgtm. I'll hold off on merging until you guys decide if you want to include quasi-literals.
I'm always in favor of not adding code until you need it...but I guess that's what this is anyway really

@cpojer
Copy link
Contributor Author

cpojer commented Dec 5, 2013

I'll add quasi-literals tomorrow so we can use all the es6 features from jstransform. The cost of adding these isn't high and I think going forward it is best to sync our transforms with jstransform.

I talked to @jordwalke and I am thinking of adding an --experimental-es6 flag to bin/jsx so our users don't need to set up jstransform themselves if they want to use 2015's JavaScript. I actually need this myself for a personal project.

@jeffmo
Copy link
Contributor

jeffmo commented Dec 5, 2013

sgtm, but the flag kinda seems unnecessary...just always include the transforms or don't. It's not going to run the es6 visitors unless it sees es6 syntax either way

@cpojer
Copy link
Contributor Author

cpojer commented Dec 5, 2013

Are we ok with shipping it like this to all the React users? :)

@sophiebits
Copy link
Collaborator

I think I'd rather have it as an option… or else eager people might start using ES6 syntax in their JSX code before we've decided as a team that that's okay. :)

@petehunt
Copy link
Contributor

petehunt commented Dec 5, 2013

+1 for the flag. It's useful but hard to make the argument that JSX is "just function calls" if we add all that.

@andreypopp
Copy link
Contributor

+1 for es6 transforms w/o flag but undocumented for some time

Though some blog posts could note about the possibility of this so people will start using jsx utility just to get es6 goodies and then they are just a one step from using JSX syntax :-)

@zpao
Copy link
Member

zpao commented Dec 5, 2013

If we're going to change the public one, I'm going to shoot down the idea of doing es6 without a flag.

Let's change the public jsx separately. We'll also need to do something so commoner knows to bust the cache when the flag changes.

@cpojer
Copy link
Contributor Author

cpojer commented Dec 5, 2013

Added template transforms to jsx-internal. Will make the changes to jsx in a separate commit.

@jeffmo
Copy link
Contributor

jeffmo commented Dec 5, 2013

@spicyj: Wouldn't your linter yell at them (probably by blowing up)? What about people using ES6 syntax when it starts shipping in browsers?

@jeffmo
Copy link
Contributor

jeffmo commented Dec 5, 2013

I'm not strictly opposed to a flag I guess -- but I would just make it opt-out rather than opt-in I guess. ES6 is just javascript. If you have a tooling need to skip over it, that seems legit. But I'd venture to guess that in most cases it either doesn't hurt or only helps to leave it on.

"should/shouldn't" use classes (or any particular feature of javascript) is also a valid concern -- but I just think it's a style concern that doesn't belong in the transformer.

Anyway -- life will probably go on either way. If we do opt-in, I'll probably just grumble about how we've decided that some javascript isn't actually "javascript" and bring it up every so often as a non-sequiter fallacy of some kind :p

@cpojer
Copy link
Contributor Author

cpojer commented Dec 5, 2013

jsx: I'll make it opt-in for now and when it is time we can agree to make it opt-out. Let's give other people some time to warm up to it.

@sophiebits
Copy link
Collaborator

@jeffmo True.

zpao added a commit that referenced this pull request Dec 6, 2013
Add more useful ES6 transforms to jsx-internal.
@zpao zpao merged commit a7f6082 into facebook:master Dec 6, 2013
@cpojer cpojer deleted the rest-parameters-react branch January 17, 2014 05:45
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.

9 participants