CS2 Discussion: Output: Class Methods aren’t Bound #4977
Comments
|
From @GeoffreyBooth on 2017-05-18 18:19 So I understand the appeal of being able to just always use Unfortunately ES2015 classes simply don’t support fat arrow methods. And if we need to choose between supporting the ES2015 So the question is what to do about bound class methods in CS2. This boils down to, essentially, what would you want such code to compile to? What would you want your |
|
From @edemaine on 2017-05-18 23:12 The main suggestion so far has been to put the assignment of bound methods within the constructor. Is compiling to this what you tried in your tests? The other thing I wondered about was replacing the method with a (here famous) |
|
From @ryansolid on 2017-05-19 04:38 I would like to give precise numbers on how wide precisely this affects in our code base but I realized it's a bit trickier. Like if we are just talking how often this appears in methods in classes it's not hard for me to estimate it outwards averaging.. I threw a spreadsheet together and got these numbers: Ultimately skipping deprecated repositories I figure sitting at around 500 classes, with 2700 non-constructor methods with 68% of them that use this. We have several static libraries especially on the server that while written at classes are basically just objects so that lowers the average a decent chunk. We've been trimming the fat, using a lot more 3rd party integrations instead of rolling our own, and migrating to more functional approaches to data mapping instead of declarative nested classes so at it's peak we were easily double that. Obviously there is non-class coffee code like scripts, configuration, data,. So I'm sure in total it makes up for less than 30% of all the coffee code However as I go I start seeing the ripples. Like use a data-binding framework like KnockoutJS. Well binding say an event handler now needs bind. So that means pretty much updating all the templates. I wouldn't be surprised other databinding frameworks would have this issue. @GeoffreyBooth asking what I think the output should be is a fair question and I am willing to spend some more time trying inheritance examples etc. I understand why ES classes are so important. Since this change is recent and I've been following as the labelled releases happen I was caught off guard since I didn't realize there was an issue in the past 2 releases where everything seemed to be working great. |
|
From @ryansolid on 2017-05-19 05:11 As far as I can tell this doesn’t work. I’m probably missing something with how the new super works. class A
constructor: ->
@testMethod = =>
console.log 'BASE'
class B extends A
constructor: ->
super()
@testMethod = =>
console.log 'CHILD'
super()With the nested bound methods in the constructor whats the recommended way to do inheritance/super calls? I tried super.testMethod as well but that doesn’t work either. |
|
From @GeoffreyBooth on 2017-05-19 07:01 @ryansolid When you say you have “2700 non-constructor methods with 68% of them that use this” do you mean 68% are fat arrow methods, or 68% are fat arrow methods that reference If the parent method is a regular class method, it works with class A
constructor: ->
testMethod: ->
alert 'BASE'
class B extends A
constructor: ->
super()
@testMethod = =>
alert 'CHILD'
super.testMethod()
b = new B()
b.testMethod() # Alerts 'CHILD', then 'BASE'But a method attached to However, take a look at this example: class A
constructor: ->
@baseTestMethod = =>
alert 'BASE'
class B extends A
constructor: ->
super()
@childTestMethod = =>
alert 'CHILD'
@baseTestMethod()
b = new B()
b.childTestMethod() # Alerts 'CHILD', then 'BASE'So what’s happening here? This also means that if these methods have identical names, the second declaration overrides the first. That’s why in your example, the second class A
constructor: ->
@testMethod = =>
alert 'BASE'
class B extends A
constructor: ->
super()
@baseTestMethod = @testMethod
@testMethod = =>
alert 'CHILD'
@baseTestMethod()
b = new B()
b.testMethod() # Alerts 'CHILD', then 'BASE'In this version, instead of calling I’m sure there are other workarounds. I’m not saying that these patterns are the best way to handle things; I’m sure @connec or someone else who knows classes better than me has better ideas. I just wanted to try to illustrate a bit what’s going on. Another interesting thing: class PostsController
constructor: (app) ->
app.get '/api/posts', @index
index: (req, res) ->
console.log @
app =
get: (route, fn) ->
fn()
postsController = new PostsController app # Logs undefined
postsController.index() # Logs postsControllerTreating One last thought: Of the bound methods that reference |
|
From @ryansolid on 2017-05-19 15:38 @GeoffreyBooth Thank you. Yeah. I actually I had a reasonable idea what was going on I just was interested to see if there was a proposed approach to it. The problem obviously is if base class methods need to call these methods they can't be named differently. As for the need to bind @ I agree in many cases when calling off the class it's fine. I was actually auditting that since I was thinking it might not be as big as I thought. But it's looking not to be the case. It's just very common for us to use event handlers this way or data-binding where we are passing a function and we've never had to bind before. It's something we have to be conscious of as the caller instead of as the definer. For my numbers those are fat arrow methods with @ references. We're pretty good at not using Fat Arrows where we don't need them. Where we don't need them we often use static class methods without Fat Arrows or pull the method out of the class and have it only live within the file. The numbers aren't even really truly representative. I wasn't counting alot of our base frameworks, things we open source etc.. which compile down to javascript at the module level before even being included in the applications. For legacy support reasons and the fact I can ignore them for now I'm not even really putting them into consideration. It might mean the inheritance example I gave in my last post isn't super common, but it otherwise definitely would be. Also there are lot of server Models that have roughly equal number of static and instance methods since they will have both versions of each method (one calling the other) so that we can perform actions with either the instance or just the model id. And I'm also counting classes that basically are all static like a C style library. So basically if you only look under the normal cases where we use instantiated classes it's more like 80% of methods, and like 90% of the class code. Maybe it's better to think of it this way. Consider classes from classic OOP language like C++. Inside the instance method @ is always that instance. These instantiated classes mostly manipulate their internals. One of the appeals of Coffeescript to people would have been that their classes act like C++/Java/C# style classes if you use the fat arrow. It works so well it has been very appealing to just write all the code around it. Classes and their solution via Coffeescript to give that the classic OOP mechanism to Javascript was one of it's initial biggest draws. I'm not coming from the perspective of someone who sees ES6 Classes and thinking we need those (which we do need to support 100%), but rather someone who was sold on the initial sell of adding familiar Class OOP to Javascript. As for my desired output I'm not sure what's ideal. But almost anything is better than what has been done. I'd just say put it back to how it was before #4530. I get the confusing nature of it but the types of mechanism this breaks are ones that largely originated from the lack of classes in Javascript to handle inheritance in the first place. As I mentioned we use Backbone Models. In fact we use them everywhere our ORM is based on Backbone Models. But we've never used initialize. Why? Because Coffeescript has classes. We just do work in the constructor, that's where we set up event handlers etc. Same with React. With Coffeescript 1 although you can't inherit ES6 classes you could use their class style with Coffeescript. Coffeescript's class syntax and patterns have allowed us to make all our code on all platforms and all technologies essentially look the same. When things didn't play nice we forked them, or made local copies converted to Coffeescript (for smaller things). To me the current solution is way worse that the problem that started it. If you told me I had to rename all my constructors to initialize I'd take that over something so conceptually breaking. This fundamentally changes how we view and use the language. I mean people don't like it don't use it. You can always make your classes without bound methods. But this just destroys a whole paradigm of developing with Coffeescript. EDIT: |
|
From @GeoffreyBooth on 2017-05-19 17:27 There was a much earlier thread where I proposed separate Simply reverting #4530 isn’t an option, for the reasons that led to that PR. If you’d like to propose a different solution, please feel free. One last option is to just keep the CS1 implementation of classes where you need that style: PostsController = do ->
ClassPostsController = (app) ->
app.get '/api/posts', @index
ClassPostsController::index = (req, res) ->
console.log @
ClassPostsController |
|
From @ryansolid on 2017-05-19 18:21 Yeah I understand the reason not to have multiple class keywords. Going to ES5 style and managing your inheritance is obviously pretty bad too and doesn't support extending ES6 classes. I guess I'm trying to understand how #4530 is actually that big of an issue. I'm more saying what led to 4530 should be considered intended behavior. That the solution doesn't fit the problem from a consumer standpoint. From what I see is the desire to support bound classes with a legacy pattern (setting initializers to be called base class) which is all but being removed from frameworks as they move to class patterns with ES6. And have likely largely already been moved out of Coffeescript implementations if they were using classes. In all cases the people with this issue could always choose not to use bound methods. That prevents the bug. The fact the bug exists is because of the desire to use bound class methods. From a language design perspective you don't want to have a hole that is inconsistent. However the explanation is simple and not any more confusing than others. Methods aren't bound before their super call. You use that to explain other syntax issues around super. Like why putting @ on a constructor argument it isn't available until after super. You can't enforce it at a compiler level which is the problem. So I see the rock and the hard place. You are stuck trying to fill the hole. But the solution doesn't even help the user with the initial use case. Pretend you are the user with this issue and you report it. You are clearly using bound class methods. The response says sorry given the restrictions of ES6 classes you can't bind before super. They'd be like that really sucks I have to change all the constructors in all my models/components. And you offer well the alternative is you can't bind anything. What do you think they'd take? It's obvious from direction this request would even come from that it's a desired feature, if not defining feature of the language. It doesn't take an exercise of pulling out percentages to know that. While I agree not everyone is using the fat arrows in the same way they are definitely using classes with @ references. We may have set the convention for it in very systematic way, but it's always been there to use and people are clearly using it (otherwise it wouldn't be a concern). When talking using client side binding libraries I have to imagine it's ubiquitous. I guess the problem with 4530 is that I understand the problem (I atleast believe I do). I see the proposed options. The last bullet point seems the obvious choice to me just looking at it. But then there is no discussion no position. No explanation. Just a bunch of yeah sounds good. So I don't feel I understand the reason for the decision because to me in flies in the face of intention of the problem. Which leaves me no forum to even really discuss it. Edit: And actually most relevantly here: #3093 (comment) From what I see am I correct that a few higher profile contributors to the project never liked this mechanism and have been looking for a way to remove it for years now because it diverges from JS proper too much as a concept. But until now Jeremy always stated it's usefulness in practice outweighed that. |
|
From @ryansolid on 2017-05-19 19:10 I guess the problem is that this sort of feature is a bit hit or miss on impact. Where people use it, it's more than "irksome" to change how it works. If you made the decision to use this the decision will likely carry through huge portions of your code. So by breaking this functionality might not affect that many people to the same extent but it will alienate certain projects almost completely. I and my team fall into the second category. I might seem dismissive about most of the proposed alternative options, but they don't really work. I don't even have to look far to see why they don't. At a certain point I'm sure enough combinations of them could. It's just rewriting everything at which point you start questioning yourself. I can respect the difficulty of the decision. Just understand for those affected it may not be minor. I highly doubt that I'm alone in this. But until this enters the more general public perception, you will likely never know. Personally I'd love to really understand how this solution made sense to people, but maybe it's pointless discussing it at this point if it's a foregone conclusion that there couldn't have been a mistake made with #4530. To me this feature should have never even been even up for discussion to go on the chopping block in the same way when you took over the project you decided there was going to be no talk of removing implicit returns etc. As far as I can tell no one had what was considered a "reasonable" solution outside of gutting the whole feature. I think there are only 2 options really the one you took, or leaving it the way it was. There might be small things to aid the latter but that's the fundamental position. I will continue for a bit trying to think of options, but truthfully I lack understanding of the compiler to be able to speak to the part of the issue that I think actually concerns you guys since it was not the use case nor the resultant ES6 that was prioritized here(as both are acceptable with the original code), but rather the implementation. I have nothing there to appeal to you with. I honestly implore everyone involved to reconsider because I think this will mean leaving it at Coffeescript 1 for a subsection of users. And they may never jump on board again. But I'm pretty sure it was wishful thinking on my part that this migration would be manageable for everyone. The early alpha/betas were really promising. It's just difficult because I feel our investment into really using Coffeescript as Coffeescript right from the beginning is why this is so much harder now. If we hadn't used it for everything, set conventions, trained in it, lived and breathed it this wouldn't have mattered nearly as much. |
|
From @GeoffreyBooth on 2017-05-19 19:41 #4497 lays it out. The problem specifically:
The most forgiving solution was discussed there:
So if we had used this solution, you would still need to refactor your code. Any base class constructors that call If you can think of another solution, by all means suggest it. You don’t need to understand the compiler; you just need to understand ES classes. Just provide a few examples of Coffee code with bound classes and what you would want them to compile to in ES. Show how your alternative compilation (as compared to 2.0.0-beta1, the last version that supported bound methods) would avoid the bugs/issues raised by #4497. If your ES solves the problem and doesn’t introduce issues of its own, by all means we would go with that instead. Bound methods were removed for technical reasons, not as a design decision. If there’s a way to keep them that isn’t buggy or introduces problems, we will keep them. This is why we have a beta period. |
|
From @ryansolid on 2017-05-20 06:25 Yeah the renaming would require zero refactoring of my code as we don't use that pattern anywhere. The constructor is the thing that already exists to hook into. Other hooks happen later. But I can't speak to everyone. But going forward with what you said I guess ideally something like this: Coffee: JS: The checkHelper is really the key to it. If we're ok with runtime errors we might have to throw an exception here to catch the bad usage. This means though that bound methods will have to be associated with their class or inherited class types and you won't be able to bind them to something random. This also is the return of the utility functions you guys were trying to remove from the language. Also if they bind the method to an event handler it won't error out right away until the event fires. So obviously better error and probably better renaming. In cases where a method is inherited from a bound method there is the overhead of the checkHelper because the prototype chain never truly cleaned up, so those undefined/instance checks get called every time you call super if the base class method is bound. But I feel those are reasonable trade offs. I hope the gist of it is clear. On the positive though this doesn't block inheritance in any sort of way. You can still call bound methods in their parents super in any context that they are still being called with this being the instance (which is the general case). This just catches when they are using it in a way that requires the fat arrow binding before super. It also doesn't break prototype chains with consecutive bound methods in child and parents. Truthfully this calling methods from the parent constructor is an awkward ES6 problem in general and not just limited to Coffeescript. If you look on stackoverflow: http://stackoverflow.com/questions/32449880/parent-constructor-call-overriden-functions-before-all-child-constructors-are-fi.. Other OOP languages throw compiler warnings about doing it. It just isn't a good pattern for classes. Whether we do something more strict and enforce it is another thing but as you know by now if I had to choose between that and 4530. |
|
From @connec on 2017-05-20 12:12 What if we compiled the prototype version of bound methods to a function that throws an exception, and put the bound function in the constructor (directly or by reference): class Base
constructor: (@element) ->
@initialise()
class Component extends Base
initialise: ->
@element.addEventListener 'click', @onClick
onClick: =>
console.log 'Clicked!', @var Base, Component, boundMethodCalledOnPrototype
__boundMethodCalledOnPrototype = function () {
throw new Error("Bound method cannot be called on prototype")
// or "bound method not ready" or "method not yet bound" etc.
}
Base = class Base {
constructor (element) {
this.element = element;
this.initialise()
}
}
Component = (function() {
var onClick
// Hoist the function out for readability - we could compile it directly in the constructor as
// with babel's public class fields plugin.
onClick = function () {
console.log('Clicked!', this)
}
class Component extends Base {
constructor () {
super(...arguments)
this.onClick = onClick.bind(this)
// or, as with babel's public class fields compilation:
// this.onClick = () => {
// console.log('Clicked!', this)
// }
}
initialise () {
this.addEventListener('click', this.onClick)
}
// We could even just leave this off, giving people an "undefined is not a function" error
// instead, but the extra detail would probably help debugging.
onClick () {
__boundMethodCalledOnPrototype()
}
}
return Component
})()This would allow 'legitimate' uses of bound methods and make the calls to the unbound version very obvious. There would certainly be some shenanigans in the compilation, hoisting the function etc. (possibly made easier by the hoisting code added for classes, maybe?). That said, I still feel that bound methods are a bit of a mistake in the language. Rather than pretend functions work this way in Javascript, I'd rather there was a bound access operator that removes the boilerplate of Another possible 'solution' to this would be to hold off until we know what we're going to do about property initializers, after which there should be a smoother refactoring path for bound methods (e.g. |
|
From @ryansolid on 2017-05-20 13:59 @connec I have to imagine Typescript must do their bound methods something similar given the constraints they have around theirs. It's a reasonable proposal. It does deal with where most of the pain comes from in practice. Although more than anything it actually showed me why my example above (the arguably simplest case) isn't actually the complete picture. If I was truly picking an example from the code at my office (although we don't use this pattern of calling in the constructor), the idiomatic definitions would look more like this: Although we don't inherit these functions called in constructors as the example we definitely inherit bound methods. I don't believe the hoisting as presented let's you do that. You wouldn't be able to call super on baseMethod as it would find the prototype method and throw the error. Although what I posted does allow that. Obviously this example will throw the error for the callback in both cases and not until the event is fired. I will take some time to read about property initializers. If they support inheritance I think that could be reasonable way forward. As much as the lack of ability to work with ES6 classes is very painful if delaying the migration by a couple years would make a big difference I suppose it could be workable. It wasn't that long ago that this wasn't even really hope, and have stuck with it this long. If there was a way to keep classes as close to their CS1 brethren obviously that would be the best from my perspective. Atleast until there is a better option (although I do see the desire to start CS2 clean if there is a better long term solution). |
|
From @connec on 2017-05-20 15:58 I see, your version would indeed catch more access patterns. Whilst I'm not a fan of bound methods, I feel your pain wrt conforming to ES2015's class semantics. You may find some of the threads in the ES2015 class PR interesting for reference. There were already a few hoops jumped to support bound methods and I take it, for your purposes, a bound access operator would not be ideal? I imagine the refactoring effort of finding and updating every call site where the bound-ness matters would be a significant undertaking. |
|
From @ryansolid on 2017-05-20 19:19 Yeah I mean I think what you guys have done given what the undertaking is fairly elegant. While it forces some restrictions it doesn't muddy the code or even really the output code. Yeah bound accessors changes where the decision happens which isn't particularly helpful. Property initializers are alright although if we are thinking of this as a direction they are much closer to the hoisting into the constructor. I think the last few years while ES6 has been getting accepted through Babel in live frameworks and the growth of maturity in those frameworks the perception around Javascript has changed a lot since Coffeescript was released. Coffeescript while always just compiling to Javascript sort of flew in the face of it a bit. And now several years later we're stuck recognizing that Javascript is the direction of Coffeescript's future. In that perspective I think that actually limits what Coffeescript can and should be doing. Bound class methods don't make sense for JS and didn't make it into ES Classes. Coffeescript used to be able to say I don't care, let's do the conceptually simpler and more familiar thing (from other OOP languages). In terms of your proposal (to hoist them) for bound methods might actually be enough in practice. If I give up that desire to pretend Javascript has something it does not (no matter how appealing it is) there are some things that could be workable. I need to try a few things over the next few hours but my thoughts are the following:
I know that I definitely have code that doesn't follow the general case of point 2. Pretty much all my REST Controllers. But they aren't deeply inherited and will never be so a change in the library base class can do a lot of the heavy lifting. Most importantly is code could be migrated to this paradigm of Fat Arrow and still work in CS1 I believe. I haven't been concerned so much about the migration at a file scope. More that it'd be immediately broken in a way that would mess up everything downstream. Something like forcing arguments in super is manageable since even if I have to update that 1200 times I don't need to code split the modules since if only one application is building in CS2, the modules still build in CS1 for everything else. I'm going to have to hit everyone of those files, but it can be a modular systematic process and one I can start before committing to migrate. Removing unnecessary Fat Arrows is similar. So I'm going to validate this approach you have suggested. Obviously my proposal is the closest to make it work exactly like CS1 classes since I even allow bound methods in the constructor as long as they aren't being used out of their instance context. This keeps maximum compatibility for bound methods and only errors out where we absolutely have to. Inheritance fully works. So it feels like all the benefits of CS classes and ES classes. I think there is a lot appealing about that (besides the fact it would require no refactoring for me). However I can see this JS purist, pragmatic perspective, that we're hacking something unnecessary on top that can never be without holes and no matter how well documented or how well handled will confuse someone. That's a tough one guys. Obviously there is the whole side of validating that what has been proposed even works completely but directionally that's hard. And I recognize there are biases, although I applaud everyones open-mindedness to atleast recognize their perspective might not be best for the language and community on a whole. EDIT: I also recognize at a certain point with hoisting as a language construct, can be deflected back to just do that yourself and leave it at no Bound Class Methods. So I have to admit I have a least a little syntax prejudice. Beyond it being more refactoring work that replacing an = sign with a - in most cases and that I can change them at my leisure as well since they will only break in very specific cases. Guys it's awkward and ugly for something would be almost used exclusively in the client. I realise property initializers gain that back but that's not a migration path and there is uncertainty. At a minimum I think making it part of the language is necessary today. EDIT2: With only addressing a small test web app with endpoints (yet to try React Native mobile) using most of the pieces @connec suggestion can work pretty well. The saving grace is a lot of our open source libraries that we write/contribute to are compile down to JS. So while conceptually looking at the source I could see places it would require more work if they interacted directly it didn't matter. Application level code never went further than 2 inheritance levels on top that. It helped that most of the client code needed to Fat Arrows so there wasn't much to change. As mentioned whether that's the right decision or not is a different thing. But if that was direction this went I couldn't really be opposed from a refactor and on the surface development experience perspective. Preferred not, but nothing to complain about. I guess in both cases it needs to be vetted any gotchas. |
|
From @ryansolid on 2017-05-23 18:56 So where do we go from here? I think mine or @connec approach would work for the majority of users for the majority of their usage. But I'm pretty sure solutions like these have been discussed before and passed over. So I'm unclear what additional considerations or criteria need to be reviewed to make this decision. It feels like more of a fundamental question about the direction of Coffeescript and how to best support the migrating the existing user base. As I said earlier the original symptom that triggered the conversation is much more trivial than the stance. It's either:
2 - 3 is a sliding scale. But is it one that needs to considered. Like with ES2015 Class PR mentioned there was quite of discussion around handling of @ constructor params assignment around super. I think the current implementation is fine, but I can think of ways to handle more of those use cases closer to how CS1 does. I'm just not sure it's worth entertaining the idea. This is sort of similar albeit more fundamental in the pattern it encouraged that the impact extends outside of the single method with the issue. |
|
From @GeoffreyBooth on 2017-05-23 19:27 What exactly is the method you tested in your comment? Can you show some examples? I think the way to go from here is to first show some examples of Coffee input and desired JS output, from simplest minimal case to most complex minimal case (i.e. most complex in terms of it Then we should consider whether getting to that desired JS output is something the compiler should do for you, i.e. magical behind-the-scenes hoisting etc., or if we should just write documentation for how people should refactor. If we decide that the compiler should do it, then someone needs to step up and invest the time in writing the PR. We’re all volunteers here, and @connec has other things on his plate, so if this is important to you then you might need to invest some time. The earlier classes PR and especially #4530 should tell you exactly where to look for what needs changing. |
|
From @helixbass on 2017-05-23 20:26 I'm not clear from reading this discussion whether it's been agreed that the current situation (ie #4530 having been merged) needs to change before This is a shockingly breaking change (especially as I've been learning React and admiring how much cleaner its passing-around-bound-method-callbacks style is using Coffeescript bound methods). I'm agreeing with everything @ryansolid is saying: That this is a hugely breaking change for the (lots and lots) of people who use this style, whether b/c they use callback-passing frameworks or they just like the set-and-forget aspect of always using bound methods That it seems to have been pushed through due to @connec's (admitted) pre-existing prejudice against bound methods That the use case in which bound methods could actually break existing code is pretty small (@GeoffreyBooth based on some of your comments I'm not sure you understand in what specific cases something breaks -- somebody correct me if I'm wrong but I believe it's only if a bound method gets passed as a callback (eg That this is comparable to The context that to me seems obvious (but I'm not sure if/where it's been discussed) is that you want to minimize breaking changes in Not clear on how the early-stage public class properties proposal relates to this, once it's deemed a legitimate compilation target then does it provide a safe way to implement bound methods (is this what @connec is suggesting above)? In which case wouldn't it make sense to keep them (as-is or via one of the proposed compilations, with the small use case where code breaks when upgrading to So the above may be unnecessary argument if it's already been accepted that the current remove-bound-methods behavior as merged via #4530 needs to change, otherwise I feel strongly that it should So then as far as the discussion of keeping bound methods but trying to compile them in a safer/more-obviously-breaking way, I don't think I'm in as strong of a position to evaluate the proposed compilations as you guys are but the fact that @ryansolid's supports calling bound methods from within |
|
From @GeoffreyBooth on 2017-05-23 22:33 To reiterate: bound methods were removed because we couldn’t find a way to output them within an ES class that didn’t introduce bugs or subtle issues. There were not removed because of some prejudice against bound methods. If you can propose a new output for bound methods that works in an ES What is needed now isn’t more imploring of how desperately you want this feature. Please write some examples like I suggested in my comment, so we can start scoping out what bound methods support in CS2 might look like. |
|
From @helixbass on 2017-05-24 00:01 @GeoffreyBooth I get that a "let's solve it" attitude is more useful than a "let's complain about it" one. But given that there may not be a suitable solution, I think it's very important to make the case (as @ryansolid has imo also done) that you guys failed to weigh the downside of introducing a massive breaking change (by merging 4530) vs introducing a class of subtle bugs. You seem to be saying that that feature must be removed b/c it introduces these bugs. To me that's highly questionable thinking, I think (as a starting point, independent from considering ways to remove/alleviate those bugs) the extent/nature of these bugs should be weighed against the downsides of removing bound methods. If you're willing to acknowledge that such a weighing should even be done, I'm arguing that you guys (yes, led by someone who's wanted to get rid of bound methods for a long time) severely underweighed requiring so much existing code to be rewritten (it feels a bit like @ryansolid and I represent the "typical user" perspective and it's a shame that you're dismissing my "imploring" on their behalf). And again, from what I can tell there's a similar class of bugs being introduced in So let's talk solutions. If by "avoids the issues that led to us removing the feature" you mean "existing code can't break at runtime in any way"/"bound methods must always be able to be called in their proper bound form" then it doesn't seem as though anyone's seeing a way to achieve this while keeping bound methods. @connec and @ryansolid's proposed compilations are both ways of making the bugs non-subtle (by explicitly erroring at runtime rather than subtly running against an unintended |
|
From @GeoffreyBooth on 2017-05-24 00:31 There are no bound methods in the CoffeeScript compiler codebase, so removing them had zero effect on that code; and that’s the only common codebase we can all refer to when judging how prevalent a certain feature is. I ran a few old projects through the CS2 compiler and had to change several uses of It’s not clear to me what your desired solution is. Perhaps you should start a new thread with a specific proposal that has examples of “this CoffeeScript becomes this JavaScript” with all the relevant cases. Just scrolling up this thread it’s not clear to me what you would prefer bound methods be compiled into. If I were to try to take this request and run with it and make a PR, I’m not sure what I would be trying to achieve. That’s what I mean by “stop imploring”—it’s not that I’m saying that I’m deaf to your pleas, but rather that I don’t know what you’re asking for. Please get specific. Write a comment or new issue that lays out exactly what you want bound methods to be compiled to, as if you were writing instructions for someone to go and implement your proposed output as a PR. Not that I or @connec will necessarily do so, but at least then it will be very clear what you’re going for and we can give feedback on it before you or someone else spends the time trying to implement it. |
|
From @ryansolid on 2017-05-24 06:39 @GeoffreyBooth to be fair I only setup a simple scenario of having a couple classes, one extending the other with all bound methods where each called it's super method. I threw it together in a jsfiddle in a few mins and moved around the calls a bit to validate if it is possible, but obviously far from exhaustive tested and likely not the cleanest. It just so happened the first way I tried to solve it seems work. But It isn't without tradeoffs. There is probably a better way to solve it. It was more just to keep the conversation going. So, @helixbass it needs a bit more work before we look at trying to implement it. I'm open to spending time to work on this further. I have been just been trying to determine 2 things:
I will do some more work assuming both the above are true to refine my solution. I hadn't buckled down on a single solution since ultimately I just want bound class methods back. It seemed there were several ways to approach it from the threads yet 4530 was accepted which has had me concerned that it was all for not. I do think it is important for myself and those coming in with a similar mindset understand that despite the disbelief that 4530 could ever be merged, especially over that edge case, it happened honestly. We aren't missing something here, no matter how incredulous this change seems. |
|
From @ryansolid on 2017-05-26 05:27 Ok, given where things are sitting I'm going to present what I consider currently the baseline fix, ie.. the simplest way to both bring back bound class methods and not have the bug occur silently. I realized while what I posted earlier might have some marginal benefits the solution could be reduced down to something much simpler to atleast talk about this class of solution. Procedurely it is taking the output pre 4530 and adding a line of code at the beginning of every bound method to check that the method is in fact bound as expected. Something like: if(this === undefined || !(this instanceof Component))
throw new Error('Bound class method accessed without being bound by super');This means bound methods can never be bound to a non-inherited context but that seems like a reasonable limitation. So Coffeescript: class Base
constructor: (@element) ->
@initialise()
class Component extends Base
initialise: ->
@element.addEventListener 'click', @onClick
onClick: =>
console.log 'Clicked!', @To JS Base = class Base {
constructor (element) {
this.element = element;
this.initialise()
}
}
Component = class Component extends Base {
constructor () {
super(...arguments)
this.onClick = this.onClick.bind(this)
}
initialise () {
this.element.addEventListener('click', this.onClick)
}
onClick () {
if(this === undefined || !(this instanceof Component))
throw new Error('Bound class method accessed without being bound by super');
console.log('Clicked', this)
}
}Or the more idiomatic example: class Base
constructor: (@element) ->
@baseMethod()
baseMethod: =>
# do some stuff with @
class Component extends Base
baseMethod: =>
super(arguments...)
@element.addEventListener('click', @onClick)
onClick: =>
console.log 'Clicked!', @Base = class Base {
constructor () {
this.baseMethod = this.baseMethod.bind(this)
this.element = element;
this.baseMethod()
}
baseMethod () {
if(this === undefined || !(this instanceof Base))
throw new Error('Bound class method accessed without being bound by super');
}
}
Component = class Component extends Base {
constructor () {
super(...arguments)
this.baseMethod = this.baseMethod.bind(this)
this.onClick = onClick.bind(this)
}
baseMethod () {
if(this === undefined || !(this instanceof Component))
throw new Error('Bound class method accessed without being bound by super');
super.baseMethod(...arguments)
this.element.addEventListener('click', this.onClick)
}
onClick () {
if(this === undefined || !(this instanceof Component))
throw new Error('Bound class method accessed without being bound by super');
console.log('Clicked!', this)
}
}I guess the question is would this sort of approach be acceptable of throwing run time errors? I'm not sure what other types of examples to show since it's always the same code. Essentially if not being bound is the problem we check for that but otherwise leave it the way it was. So we don't impose any additional constraints outside of the can't be bound to other contexts. However, bound methods can fully inherit and be inherited and called as normal by base classes including in their constructors. |
|
From @connec on 2017-05-26 11:04 I like that approach. It's simpler than my proposal but has the same outcome. The only thing I'd suggest is that the conditional throw should be wrapped in a helper similar to babel's function _boundMethodCheck(instance, Constructor) {
// note that `=== undefined` is redundant, since `undefined instanceof` is safe
if (!(instance instanceof Constructor)) {
throw new Error('Bound instance method accessed before binding')
}
}
...
onClick () {
_boundMethodCheck(this, Component)
...
} |
|
From @GeoffreyBooth on 2017-05-26 15:55 I know we generally try to avoid runtime checks and runtime errors, but I don't know how absolute that rule is. @lydell, do you care to comment? I assume that a runtime check, especially a tiny one such as is proposed here, is preferable to removing this feature or letting it live in a dangerous (unchecked) form? Assuming I'm correct on that, who wants to try to implement this as a PR? |
|
From @lydell on 2017-05-26 16:04 I haven't been following along here, because bound class methods don't interest me very much and the comments have been so long. But it sounds like you've reached consensus on a way to implement them, so I'd say go for it! I don't mind the little |
|
From @mrmowgli on 2017-05-26 16:14 +1 |
|
From @ryansolid on 2017-05-27 03:33 @connec that looks good. I also realize the limitation I was talking about in my post was imagined since a function can only be bound once anyway. So looks good all around. @GeoffreyBooth thanks for your patience. |
|
From @GeoffreyBooth on 2017-05-27 03:38 Happy to help. @ryansolid, do you want to take a crack at this? Unless @connec you feel inspired? |
|
From @rattrayalex on 2017-05-27 04:10 Having a stacktrace point directly at the method might be helpful to developers (which is to say, inclining the check could be nicer). Either way looks good though. I'm a major +1 to the feature overall fwiw. |
|
From @helixbass on 2017-05-27 13:34 @GeoffreyBooth I'll try and implement this unless @ryansolid or @connec prefer to |
|
From @GeoffreyBooth on 2017-05-27 15:38 Thanks @helixbass. I think we want the helper. It's more DRY and in keeping with our style. It just means that the offending method is in the second line of the stack trace instead of the first. |
|
From @GeoffreyBooth on 2017-06-06 15:20 Is this in progress? We'll be releasing a new beta soon with the JSX PR, it would be nice to include a fix for this. |
|
From @helixbass on 2017-06-06 20:21 @GeoffreyBooth I have this working on my One thing I ran into was how to handle anonymous classes (since the helper expects a class name/reference as one of its arguments) - the options I could see were (1) disallow bound methods in anonymous classes (2) forgo the runtime check for anonymous classes or (3) give anonymous classes with bound methods a name. (3) might make the most sense but I wasn't totally clear on what was going on with eg |
|
From @GeoffreyBooth on 2017-06-06 21:11 Forgive my ignorance if this is a stupid question, but:
|
|
From @helixbass on 2017-06-06 21:34 @GeoffreyBooth from my understanding, yes, the edge cases can only happen when a parent class's In my implementation I followed what @ryansolid and @connec outlined above, which is that every bound method (of a base or derived class) performs the runtime check, passing its own class (not the parent class) as the second arg (ie However if our understanding is correct and this issue only can happen for child class bound methods, then your suggestion makes sense and we could (1) pass the parent class as the To be thorough/pedantic, there's already the edge case (within the edge case, ie only in a parent class constructor) where a bound method could be called with the wrong |
|
From @GeoffreyBooth on 2017-06-06 21:45 I’ll be honest I don’t quite follow everything you wrote, but my point was just that if it’s not possible to |
|
From @helixbass on 2017-06-06 21:59 @GeoffreyBooth basically I think you're right that b/c it only applies in child classes we can just always pass the parent class to the check (so yup I think your suggestion resolves the anonymous class question nicely by making it irrelevant). I'll optimistically update the PR to behave this way but please @ryansolid @connec or whoever chime in if you think it should remain as specified ie check against its own class and check for all bound methods not just child class bound methods |
|
From @ryansolid on 2017-06-07 03:53 Right so classes that aren't extended don't need the check. That makes sense since there is no issues around the timing of calling super. And since a child of a child is still an instance of the Base I don't see any issues. If the base class bound method is overridden by a bound or unbound version that base class will still call bind on the method before any chance of the child method being called. And since the method can only be bound once that checks out. |
|
From @GeoffreyBooth on 2017-06-30 17:41 Hopefully fixed via #4594 |
From @ryansolid on 2017-05-18 08:50
I think this belongs here as it’s more of a directional thing than say a bug. I’ve read #4530, but on the surface I didn’t realize how breaking this change is. It’s not just replacing the syntax. The methods aren’t bound. Like pretend you are writing a controller in Express.
@is nowundefined. Now I realize I could have written@index.bind(@). But that is the whole convenience of the Fat arrows. Whole patterns/conventions/libraries in coffeescript are written around this. It’s a set and forget and you don’t have to worry about tracking@. Once you are forcing people to use bind you are back to thinking about tracking@. Now it’s possible I’m not understanding something, but this seems unacceptable from the point of view of the initial goals of this project.Edit:
While the syntax change could be considered on par with say changing the direction of the spread operator. To me the removal of the ability to bind class methods is more on par with say getting rid of implicit returns. Sure I could go and add return everywhere and rewrite my loops to support it but is that really coffeescript.
Edit2:
I suppose I’m considering this from the perspective of the so called heavy use case. I don’t know how common this is. I learned to use coffeescript this way 5 years ago. In the company I work at pretty much every coffeescript file is a class with bound methods from our components, models, stores, static libraries, and controllers on the web clients, mobile, and the web server as well as a large majority of the files in our backend processing/jobfarms/message queues. This spread over hundreds of modules and hundreds of thousands lines of code. Saying it’s a pain to refactor is an understatement. It basically redefines the language.
Out of the presented solutions making constraints around the use bound methods around super seems like the obvious solution. I say this from the perspective of using both Backbone and React and frameworks which extend base classes in coffeescript as part of the aforementioned code base. It’s possible I am alone on this but given the tools coffeescript gives I find that hard to believe.
The text was updated successfully, but these errors were encountered: