Sugar
Sugar copied to clipboard
andrewplummer
Proactively unwrap chainables when return value is a boolean.
via Twitter from @ioquatix
Version 2.0.0 added chainable behavior, however one note I immediately got is that the behavior of having to unwrap boolean values, either explicitly using the raw value, or implicitly using == is counter-intuitive. For example:
var date = new Sugar.Date('tomorrow');
if (date.isValid().raw) // Explicitly unwraps chainable
if (date.isValid() == true) // Implicitly unwraps chainable with "valueOf"
if (date.isValid() === true) // Does not work as the chainable is not unwrapped
if (date.isValid()) // Does not work as the chainable is not unwrapped
As Sugar's date module is often seen on it's own, users who are only interested in dates (such as this user) may not (should not have to?) understand the chainable behavior.
This could potentially be improved by separating methods that return booleans and defining different behavior on them (i.e. always returning an unwrapped boolean). Note particularly that this would make Sugar more intuitive when compared to moment.js, which doesn't have to deal with this issue as it is only concerned with dates.
However this has a few possible issues:
- It could potentially be more confusing as it breaks the promise that a chainable method will return a new chainable. Would this become "returns a new chainable unless the result is a boolean"?
- It would differ from Underscore/Lodash whose
_.chainmethod do essentially the same thing as Sugar does now by wrapping a raw value, and do not unwrap boolean types either. I'm not against this but it would need a decent justification. - The assumption here is that there is no value in chaining booleans, but is this actually true? Although it's unlikely that there are any boolean-specific methods would be particularly useful, a wrapped boolean still has type checking methods such as
isArrayetc available to it. If the return type is unknown for whatever reason, these methods may still be useful. - What about methods that return booleans or
undefined(if any?). As a tangential issue, what about methods that returnundefinedornull? Should these be unwrapped too?
Give it a thumbs up if you like it! I'm going to be looking at these.
After considering this, one thing I really don't like about the idea is the potential for confusion about which methods are unwrapped. If all methods were prefixed with is, it may be easier to explain, however there are other methods that return booleans which cannot follow this pattern as they are maintaining parity with native methods. For example:
var d = new Sugar.Array([1,2,3]);
d.every(1);
I think it would be very confusing for this method to be unwrapped and return a boolean as it isn't following the naming pattern that would indicate this. However, if we don't unwrap it selectively for methods that don't follow the is pattern, it could potentially be more confusing, and the original issue raised would still exist.
Any thoughts?
I think never auto-unwrapping is the more consistent solution. Let's not create Yet Another List Of Special Methods that everyone has to remember...
I think that I agree with this. I want subsequent versions of Sugar to have a lower hurdle to their use, not higher. That said, the core issue I would like to address here (if it's even possible) is also a hurdle of sorts:
var d = new Sugar.Date('today');
if (d.isValid()) {
// ...
}
If someone was introduced to Sugar and saw just this code, when compared to other date libs like moment.js it is unfortunate that you need a timeout to address why isValid() doesn't just work as you would expect. Even if wrappers and chaining are standard in Lodash, etc, you may not know about this or even if you did it may not be immediately obvious.
I wonder if this can't somehow be handled without creating more overhead for everyone else. I'm beginning to think that it can't, but I at least wanted to state the root of the problem here first...
I agree. There is also the implicit performance costs associated with wrapping everything, e.g. a memory allocation, a method call to get the actual value, etc. It might also mean that the optimiser has a harder time doing the right thing.
Everything here is very reasonable and well thought out. I agree with all of it.
However, I still think the original issue should be solved in a way which doesn't violate the principle of least surprise.
If you wanted a rule, perhaps this is it:
For methods that return "composite" objects, e.g. arrays, dictionaries, dates, objects, the object can be chained. But for simple values, e.g. numbers, booleans, strings, the value is not chained by default.
Ah ok I see...
Well, the problem with that rule is that Sugar provides many useful number and string methods. Unwrapping these would inevitably lead to people needing to rewrap the result, which would lead to chaos. Even booleans could potentially have other methods on them, although it's hard to imagine what they would be. However, for example even undefined would find use in staying wrapped, if it wasn't known if the value is undefined there are object type methods like isNumber, etc.
I'm pretty sure 99% of use cases of isValid() is going to be if (date.isValid()).
Well I agree with that part for sure. Just to throw it out there, here are the current methods that begin with "is":
-
Date#isUTC -
Date#isValid -
Date#isAfter -
Date#isBefore -
Date#isBetween -
Date#isLeapYear -
Date#is -
Number#isInteger -
Number#isOdd -
Number#isEven -
Number#isMultipleOf -
String#isBlank -
String#isEmpty -
Object#isEqual -
Object#isArguments -
Object#isMultipleOf -
Object#isEmpty
Also language methods like:
isHangul, isDevanagari, etc.
...and type methods like:
isNumber, isObject, etc.
Edit: also date methods like
isLastWeek, isNextWeek, etc. Actually there are a lot more like this.
So you propose to make all isFoo methods return unwrapped boolean? I think it's a good idea.
I think that if this were to get implemented, this rule ("begins with is") is the only way to do it without introducing major confusion. As mentioned though, I don't love this idea because it's still one arbitrary rule to remember. It's just a question of where we introduce it. It's either:
- Chainables need to be unwrapped with the
rawproperty, where the gotcha comes when you attempt to use a chainable. Frankly I'm of the opinion that users not approaching from the date angle will not have an issue with this, but those who are used to momentjs etc might.
or
- Any method that begins with
iswill be automatically unwrapped. The assumption here is that this will be intuitive and that users won't be expecting these methods to be wrapped. However this is an assumption and it will be a gotcha for some users. The hope is just that it won't be that many and that it won't be an initial hurdle for use.
I'm starting to think it's the chainability that needs to be made explicit ;-)
@vendethiel This is the stance that I'm tending to lean toward as well. It's worthwhile noting that the complaints that I have had come in came before I created the Date page was created. I'm feeling now that it would be better to have better pointers to that page and tighten it down even further to really solidify the concept as a first step to working with Sugar. Keep in mind that static methods return booleans so there is still a way to code around this for people who really don't like it, and it doesn't even have to be long:
_d = Sugar.Date; // This could be a project wide shortcut.
if (_d.isValid(date)) {
// this will still work as expected
}
Also, extended mode is still available which, if allowed, solves the issue and works as expected here. Even people with an aversion to modifying the global state may be willing to make an exception with the Date object, and this is easily supported with Sugar.Date.extend() (selectively extend only date methods).
Lastly, one note is that a quick audit of the Sugar shows that the only methods that do not start with either is or has are following a parity of their own. These methods are:
-
Array#some -
Array#every -
Array#none -
Object.some -
Object.every -
Object.none
some and every are maintaining parity with their ES5 counterparts, and none is intended as an inverse of every. However, in addition there are other polyfill methods such as Array#includes that also return booleans. These don't apply to chainables currently, however they may in the future (#577).
@vendethiel To clarify, were you referring to documentation or something else?
Also, extended mode is still available which, if allowed, solves the issue and works as expected here.
Oh, I already know i'm going to keep using it ;-).
@vendethiel To clarify, were you referring to documentation or something else?
Well, the non-documentation part is already "explicit", insofar as extended doesn't need this behavior.
Though I guess you could argue for a Sugar.Date._(a) to get the chaining behavior...
@vendethiel Well, it's true that something like Sugar.Date.chain(a) would be more explicit (even Sugar.chain(a) would work, now that I think about it...). However it would essentially make the syntax Sugar.Date(a) useless, as it would simply return the same thing as the static methods, and it would be simpler to write:
Sugar.Date.format(d);
rather than:
Sugar.Date(d).format();
ah, that's true. Then it's just a documentation issue, explained like you do here. It might make unwrapping a non-issue "just use the static version otherwise"
I would appreciate an easier api.
E.g. I got the following dates.
date1 = Sugar.Date('today');
date2 = Sugar.Date('tomorrow');
Then I want to compare them like this:
if (date1.isBefore(date2)) {
// do something
}
But instead I have to write:
if (date1.isBefore(date2.raw).raw) {
// do something
}
Am I missing something? Is there already an easier way?
@Catscratch I agree. This is something that I will look into very soon. To refresh since this issue has been open so long though, how do you think it would be best to handle the expectation of whether or not the chainable will be unwrapped? In other words, I realize that you expect isBefore to be unwrapped and addMinutes to not be, but what kind of rule can be applied that will make this not frustrating in less clear-cut cases? Methods that start with is was my first idea but unfortunately JS does not follow this convention, so for example sugarArrayChainable.every would unwrap or not? I really would like a way to do this so that people are not having to go to the documentation all the time.
@andrewplummer Hm, good question.
I think the Java time api is a good example of how it should look like. I would only use the chainable api on functions that change the object itself but not on any comparision. Isn't it possible to directly return a boolean for every "is" function?
Also it is a really strange behaviour that e.g. addDays(1) not only gives me a modified object with one day added, but also changes the original object.
E.g. in Java you can use addDays(...) and get a copy of the original object as return value. To avoid this in Sugar I have to do something like Sugar.Date(new Date(origDate));
Well it is possible but again every also would do this as it returns a boolean but is not an is function. I would like a rule that is 100% predictable if possible... it may not be.
As for immutability I personally see the value of it but the #1 tennet of Sugar is to coexist with the native Javascript API, which is, for better or worse, mutable. So if date.setMinutes will always be a mutable method then date.addMinutes not being so would cause considerable confusion. I understand how it comes across as strange though.
As for the last one, all Sugar chainables have a clone method, so you could do sugarDate.clone().addMinutes()