Separates decorator/instance usage of disposeOnUnmount #671
Conversation
| @@ -38,9 +34,16 @@ export function disposeOnUnmount(target, propertyKeyOrFunction) { | |||
| ) | |||
| } | |||
|
|
|||
| // decorator's target is the prototype, so it doesn't have any instance properties like props | |||
| const isDecorator = !target.hasOwnProperty("props") | |||
There was a problem hiding this comment.
Very fragile check. What if in future react devs will decide to move props to prototype?
There was a problem hiding this comment.
I agree that it isn't the best check ever but I don't know what else we can do to discriminate usage. Also, props is pretty much the one property that will always be tied to the instance (props on the prototype would mean that all instance would share the same props, which completely defeats their purpose), so I think that, even in the future, we're fine making that assumption. Moreover, since classes are pretty much "legacy" now, I doubt anything will change for them in the future, and even if this for whatever reason change, we could still change this check for something else at that (very improbable) moment.
Anyway, now that v6 is out, this PR is pretty much obsolete for me (I had hoped that it could have been merged before) since the new restrictions on the method/decorator are breaking a good chunk of my uses for it. I'll still try to update it when I have the time though
There was a problem hiding this comment.
Merging this one now, since it is relevant again after 6.10 :)
Replaced this specificy check with typeof propertyKeyOrFunction === "string", since only when used as decorator we get a string here.
There was a problem hiding this comment.
This was my first idea too (and the one I thought about when I wrote the issue), but nothing prevents you to use disposeOnUnmount(this, "myProperty")... though I agree this isn't the way it's supposed to be used.
Anyway thanks a lot for merging the PR!
|
@JabX do you want to close this PR if it's not relevant anymore? It should be rebased otherwise. |
|
Releasing as 6.1.1 |
|
Fair point! Will keep it in mind if it ever comes back to haunt us :)
…On Wed, Jun 19, 2019 at 11:49 PM Damien Frikha ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/disposeOnUnmount.js
<#671 (comment)>:
> @@ -38,9 +34,16 @@ export function disposeOnUnmount(target, propertyKeyOrFunction) {
)
}
+ // decorator's target is the prototype, so it doesn't have any instance properties like props
+ const isDecorator = !target.hasOwnProperty("props")
This was my first idea too (and the one I thought about when I wrote the
issue), but nothing prevents you to use disposeOnUnmount(this,
"myProperty")... though I agree this isn't the way it's supposed to be
used.
Anyway thanks a lot for merging the PR!
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#671?email_source=notifications&email_token=AAN4NBCXWLDQ7MEG24MBGSLP3KSWNA5CNFSM4HIGIZZKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB4CQ67I#discussion_r295538609>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAN4NBFME5FDTMV6ONNPIELP3KSWNANCNFSM4HIGIZZA>
.
|
Fixes #666 by splitting the disposer store into two separate prototype and instance stores, depending on the target parameter.