Fix cloneElement type definition #464
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
is there anything blocking this PR @chenglou @rickyvetter? right now I don't think that anybody is able to use |
chenglou
added a commit
that referenced
this pull request
Aug 31, 2019
See https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/relatedTarget#Return_value This is technically a breaking change, however small. cc @rickyvetter Note that we can't pretend tiny breaking changes are not really breaking; if you drag in a third-party library that uses `relatedTarget`, you can't even upgrade your own ReasonReact because that thing relied on the old version. You're now deadlocked. So "why not just follow the types and refactor your code" isn't a viable suggestion. Refer to my Reason Conf talk and calculate the probability of breakage if you drag in e.g. 5 React helper libraries. People underestimate compound probabilities. The proper way of upgrading folk is like this: - Keep the old `relatedTarget` external the same, add a deprecation warning to it. - Make a new binding, e.g. `relatedTarget2`, with the new signature. - Wait a few months until userland upgraded. Note that there'd be _no_ deadlock here, unlike the situation the previous paragraph described. - _Then_ make a breaking release that removes `relatedTarget`, assuming all of the user's dependencies have been upgraded, so the user can truly "just follow the types and refactor their _own_ code now". Unfortunately you'd be left with the name `relatedTarget2`, which you need to swap back into `relatedTarget` in a future (still non-deadlocking) mini breaking change. I've circumvented this last step several times before in ReasonReact, by piggy backing on top of the fact that we needed to change the module name anyway; e.g. when we transitioned from `ReactEventRe.re` to `ReactEvent.re`, I took the occasion of changing some bindings in the latter and not touch the former. Aka both modules might have a binding called `stopPropagation`, with different signatures. No need for `stopPropagation2` just to avoid name clashes. We're not gonna do this process here because: - The above proper way of doing it is elaborate, and since we're not switching whole namespaces anytime soon, asking folks to use `relatedTarget2` is slightly whack. The console tells you the deprecation when you use `relatedTarget`, but someone internally mucked with the warning flags and... I don't wanna go into this *grumbles*. - We'll already be having some other breaking changes anyway (e.g. #464), so might as well do it the easy way and change `relatedTarget`'s signature directly. We try to batch breaking changes together to minimize churn.
|
Thanks for the ping. Nothing blocking, but this is a breaking change we'll have to batch with other changes. It's breaking unless you use I know realistically most people probably just wing it with |
rickyvetter
pushed a commit
that referenced
this pull request
Apr 20, 2020
See https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/relatedTarget#Return_value This is technically a breaking change, however small. cc @rickyvetter Note that we can't pretend tiny breaking changes are not really breaking; if you drag in a third-party library that uses `relatedTarget`, you can't even upgrade your own ReasonReact because that thing relied on the old version. You're now deadlocked. So "why not just follow the types and refactor your code" isn't a viable suggestion. Refer to my Reason Conf talk and calculate the probability of breakage if you drag in e.g. 5 React helper libraries. People underestimate compound probabilities. The proper way of upgrading folk is like this: - Keep the old `relatedTarget` external the same, add a deprecation warning to it. - Make a new binding, e.g. `relatedTarget2`, with the new signature. - Wait a few months until userland upgraded. Note that there'd be _no_ deadlock here, unlike the situation the previous paragraph described. - _Then_ make a breaking release that removes `relatedTarget`, assuming all of the user's dependencies have been upgraded, so the user can truly "just follow the types and refactor their _own_ code now". Unfortunately you'd be left with the name `relatedTarget2`, which you need to swap back into `relatedTarget` in a future (still non-deadlocking) mini breaking change. I've circumvented this last step several times before in ReasonReact, by piggy backing on top of the fact that we needed to change the module name anyway; e.g. when we transitioned from `ReactEventRe.re` to `ReactEvent.re`, I took the occasion of changing some bindings in the latter and not touch the former. Aka both modules might have a binding called `stopPropagation`, with different signatures. No need for `stopPropagation2` just to avoid name clashes. We're not gonna do this process here because: - The above proper way of doing it is elaborate, and since we're not switching whole namespaces anytime soon, asking folks to use `relatedTarget2` is slightly whack. The console tells you the deprecation when you use `relatedTarget`, but someone internally mucked with the warning flags and... I don't wanna go into this *grumbles*. - We'll already be having some other breaking changes anyway (e.g. #464), so might as well do it the easy way and change `relatedTarget`'s signature directly. We try to batch breaking changes together to minimize churn.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.