-
Notifications
You must be signed in to change notification settings - Fork 347
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
Fix cloneElement type definition #464
Conversation
is there anything blocking this PR @chenglou @rickyvetter? right now I don't think that anybody is able to use |
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 |
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.
No description provided.