Skip to content
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

Deprecate onChange & introduce onUpdate #58

Closed
nikgraf opened this issue Jul 5, 2015 · 5 comments
Closed

Deprecate onChange & introduce onUpdate #58

nikgraf opened this issue Jul 5, 2015 · 5 comments
Assignees
Milestone

Comments

@nikgraf
Copy link
Owner

nikgraf commented Jul 5, 2015

Initially we were striving to build components that behave exactly like HTML native ones, but with additional features & more control over them. This idea was mainly driven by React's concept of "Learn once, write anywhere". This would have included getting all the events right. So we aimed to implement the onChange callback to return an object that would look like:

{
  target: domNode(with value),
  currentTarget: domNode of current target,
  …
}

At the React Europe conference I talk to some of the React core contributors what they would recommend. The feedback was not trying to mimic the current API. One reason is that after the 0.14 beta announcement we see React going in a direction with a React core & different packages to render to different targets. We might even see a future where components work for multiple targets. This would mean the API should be designed in a way to work on different targets. Putting DOM events in there would be the wrong direction. Another reason is that I don't even know if it's possible to get it right in all the browsers. It's more of a gut feeling, as I haven't really tried apart from the target property.

After Jyoti & I decided we won't mimic the API we needed to come up with our own that can be applied consistently. We didn't even need to think about it. It was obvious the changed value is the relevant information. Luckily we got another good advice on the conference: "Whenever you create an event like callback wrap it into an object. You never know what you might want to add in the future and you don't want to break the API". While we won't add anything from the beginning I already can imagine how beneficial this advice was. We ended up with this structure { value: value }.

Last but not least we needed a name for the callback. OnChange came naturally as we already had it in the code. Unfortunately there was an issue. We didn't feel comfortable using the same name as React does, but applying different behaviour. As a developer coming from HTML/JS you already had to learn that onChange behaves differently than in the specs. Should we break with this behaviour once again? I wasn't happy about it. Even someone from the React core team told me something like: "Maybe it wasn't the best choice we picked the name onChange". So we saw an opportunity here. If we don't mimic the original API, we don't want to add confusion. So we decided to go with onUpdate. An alternative name would have been onModify. If you think that would have been better for some reason let us know.

The final API:

this.props.onUpdate({ value: value });

Did we miss something? Should we do it in another way? Let us know what you think?

@nikgraf nikgraf self-assigned this Jul 5, 2015
@nikgraf nikgraf added this to the Release milestone Jul 7, 2015
@jpuri
Copy link
Collaborator

jpuri commented Jul 9, 2015

I think we should have a page in docs that explains how we handle value, i.e how is value, valueLink, defaultValue handled and what should users expect to get from onUpdate method.

@nikgraf
Copy link
Owner Author

nikgraf commented Jul 9, 2015

👍 We probably should use the same wording a React documentation & also refer to it with links.

@jpuri
Copy link
Collaborator

jpuri commented Jul 9, 2015

Lets create a different section in docs 'guides' after component.
These details and more can be added to this section.

@jpuri jpuri assigned jpuri and unassigned nikgraf Jul 9, 2015
@jpuri
Copy link
Collaborator

jpuri commented Jul 11, 2015

I think we are done with all changes that were need for this issue.

@nikgraf
Copy link
Owner Author

nikgraf commented Sep 23, 2015

Interesting issue: facebook/react#4751

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants