Skip to content

Add es6 option to generator #332

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

Conversation

borisrorsvort
Copy link
Contributor

Here you go #316

I wanted to add some tests but basically it's just rerunning the same with another option. Should I dedupe the current one just for es6 or something else?

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@rmosolgo
Copy link
Member

rmosolgo commented Aug 9, 2015

Yes, could you add some tests for the generated output? That will help us keep from breaking things down the road :)

@borisrorsvort
Copy link
Contributor Author

@rmosolgo Good enough?

@rmosolgo
Copy link
Member

Sorry, I guess I should have been more specific. I think it's important to add tests for new behaviors, but not important to test shared, pre-existing behaviors. In this case, it would be good to test:

  • The resulting file uses ES6 syntax (eg, checking for the class ... extends syntax in the result)
  • The resulting file assigns propTypes after defining the class (eg, checking for {className}.propTypes =)
  • The resulting file compiles successfully (I think test "generates working jsx" is sufficient for that).

In my opinion, it's not important to repeat the tests for filename, render function, and propType since those behaviors are unchanged by the --es6 option.

How does that sound?

@borisrorsvort
Copy link
Contributor Author

@rmosolgo here you go

@rmosolgo
Copy link
Member

💸 Thanks, that's great!

rmosolgo pushed a commit that referenced this pull request Aug 19, 2015
@rmosolgo rmosolgo merged commit 8178716 into reactjs:master Aug 19, 2015
@borisrorsvort
Copy link
Contributor Author

👍

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.

None yet

3 participants