Skip to content

Conversation

@tannermares
Copy link
Contributor

@tannermares tannermares commented Apr 17, 2017

Why this was needed

Looks like findDOMNodes has a typo in the argument name here. selector should be searchSelector. Since the incorrect value is used ALL components are unmounted when findDOMNodes is called from unmountComponents

How I fixed it

Updated argument name from selector to searchSelector here

I adding a spec. I'm not familiar with MiniTest or server side rendering and tried to implement the failure in the simplest way possible.

I believe the failing specs are from JS warnings the rake react:update caused. Looks like maybe the createClass deprecation? Should I not rake react:update?

Any suggestions on that front and I'll gladly implement. :)

Note: This is my first contribution and couldn't find a "how to contribute" section. I apologize for any missing steps or documentation.

@tannermares tannermares reopened this Apr 17, 2017
@rmosolgo
Copy link
Member

I think you're right about react:update causing the issues. This library isn't ready for the latest version!

I don't want to merge the failing specs, but either of these three is ok with me:

  • Correct the typo in react_ujs/index but don't modify the test suite
  • Correct the typo and figure out how to make the proper test case pass
  • Correct the typo & test it, and update React and make the tests pass (shudder)

Does one of those sound good to you?

@tannermares
Copy link
Contributor Author

@rmosolgo updated

@rmosolgo
Copy link
Member

🤘 Thanks! I'm going to try to get #697 in before a release tomorrow morning.

@rmosolgo rmosolgo merged commit 5a6612a into reactjs:master Apr 18, 2017
@tannermares tannermares deleted the tm/fix-variable-typo-in-findDOMNodes branch April 18, 2017 02:23
@rmosolgo
Copy link
Member

🚢 in 2.1.0!

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.

2 participants