Fix Destructuring Errors in Util #2171
Conversation
|
the use of this allows those methods to be extended and overridden, and works as intended |
|
you can also just bind this as the Util Class again after you destructered so it works as intended |
|
But that would completely defeat the purpose of destructuring it in the first place. You might as well have just used it off the class variable in the first place. When you destructure something you're breaking it away from the context that it was created in, so why would you want to re-add that context back to it? And I didn't think of that Appell. Although I'm not sure there's a reason that would warrant you to override |
|
Yes but the Util methods are mostly private and not made to be used by you anyways so you should mostly not use methods in there since changes from private methods aren't in chaneglogs as example. (because they are private) aswell as they could get removed at any time they are't needed anymore. |
|
Just because they are private doesn't mean they shouldn't be used though. There's definitely a use-case for the first of the two. The second one I only added to be consistent. Not that its like impossible to implement it on your own for your own use of course. |
|
wait what, they are private for a reason 🤔 and also if you know that these methods use the |
|
They're private because they're not useful to the user base. That doesn't mean a developer working with the library might not find it useful to use it. And if it's such an issue about it being private/hidden, then there would never be a reason for anyone to overwrite or extend it in the first place and they should just use the class reference instead of the Edit: Honestly the only reason this PR should be an issue is because of Appells comment or the fact that this would break the consistency with some of the other classes that use the context vs class reference. Private or not shouldn't be an issue here. |
|
if you're really concerned about binding, the appropriate solution is to rebind the methods in the constructor edit: nvm i just realized they're static |
|
Well you are right tho, but still i wouldn't recommend using this class since private methods might be edited/removed in future releases, but beside that there is no real reason to not merge it since you dont really need to extend an abstract class like this |
|
What do you mean rebind in the constructor? I'm talking about changing it so it can be destructured properly... what constructor is there to rebind in? @Dev-Yukine And I understand where you're coming from, but that should be an issue left up to the developer themselves, even more-so if they take it upon themselves to use something that wasn't documented publicly. |
|
I also didn't really look at any of the other static functions that use context over class reference, I kind of just looked over them. There might be room to change those as well since most of them seemed like they were in private classes themselves, if consistency is an issue that is. |
|
I'm confused about why this is an issue. The methods you've modified work perfectly fine when destructured. If you're not rebinding them when calling them in different contexts, that seems like an issue on the part of the developer.
|
|
Your test case doesn't touch where destructuring outside d.js: |
Please describe the changes this PR makes and why it should be merged:
Since the context of a static function is the same as referencing the class variable, there's no reason to use
thisinside of it. This also makes it so that you can destructure them from the Util class and they will still work as intended. Mostly a developer standpoint here since the methods are private and would really only be used by someone who knows what they are doing.I don't think this follows any of the versioning guidelines below.. so just going to leave these blank
Semantic versioning classification: