Skip to content

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.

3 participants