Opened 8 years ago

Closed 7 years ago

#13298 closed enhancement (wontfix)

Restrict has("...") to return booleans

Reported by: ddumont Owned by: Kris Zyp
Priority: high Milestone: 1.8
Component: General Version: 1.7.0b1
Keywords: Cc:
Blocked By: Blocking:

Description

The has.js spec says that tests 'should' assign booleans or functions that evaluate as booleans, however it's possible to assign anything at the moment, and read it out later. I'm proposing we return the value of a has condition to be a boolean based on the truthiness of the return value, unless specified as undefined.

Patch file supplied.

Attachments (1)

has.js.patch (669 bytes) - added by ddumont 8 years ago.

Download all attachments as: .zip

Change History (19)

Changed 8 years ago by ddumont

Attachment: has.js.patch added

comment:1 Changed 8 years ago by bill

Owner: set to Rawld Gill

I assume the current behavior is a feature, not a bug, as it allows code like

if(has("ie") == 6){
...
}

which will be filtered out if we have browser / browser version specific builds like for webkit, IE8, etc.

I agree that it's confusing gramatically, as "has" in english implies yes/no.

comment:2 Changed 8 years ago by dante

we had this discussion in has.js early on, I was the decider on "should return a bool" ... this was to keep the optimizer code easy ... so the whole has(...) part could be replaced with a boolean and deadpath removal would just work. if the optimizers in the wild can handle simply replacing has(..) with whatever value is in the profile cache, I'm +1 with not enforcing the bool return value.

if(has("ie") == 6){

}

becomes from hasMap: { ie: 6 }

if(6 == 6){

}

comment:3 in reply to:  1 Changed 8 years ago by dante

Replying to bill:

I assume the current behavior is a feature, not a bug, as it allows code like

if(has("ie") == 6){
...
}

which will be filtered out if we have browser / browser version specific builds like for webkit, IE8, etc.

I agree that it's confusing gramatically, as "has" in english implies yes/no.

the point of has() was to avoid browser specific branching anyway. has(ie) == 6 should be has(whateverbuginIEmakesthisbranchNecessary), backed up by a small feature test. So I can see both arguments. Either a has() test does or does not pass. Storing some value in the cache is a nice inadvertent feature, and I'm OK with updating the has.js spec to be more permissive in the return values, but won't adjust any of the existing "official" has.js feature tests to return non-bool values.

as dojo is the first/only official adopter of has.js, the implementation of the optimizer wins over my vaporware optimizer, and we can adjust the spec as needed for sane implementors (much like is going on w/ AMD)

returning a truthy value is the only requirement, so has(..) can be replaced with constants, be they 'false' or 'true' or '6'

comment:4 Changed 8 years ago by bill

Note that (IIRC) the optimizer you are referring to is google's closure compiler.

comment:5 Changed 8 years ago by Rawld Gill

I don't see the need to restrict has tests to boolean responses. Also, we're using has for general branch trimming, far beyond just browser feature tests. It's a really sweet little API.

Here's how the optimizer works in the dojo build app. First, you provide a set of static has values. The current default values are here:

https://github.com/dojo/util/blob/master/build/v1xProfiles.js#L61

Currently, the build app replaces has(<feature-name>) with 0 or 1 if either a 0 or 1 is given in the staticHasFeatures build config switch. For example, say we have:

staticHasFeatures = {
  "my-feature":0
};

Then in the code we have

if(has("my-feature")){
  //do something
}

Then the builder will rewrite this as

if(0){
  //do something
}

And from there, the closure compiler will trim the branch. (Note: shrinksafe will not trim this; that's why we need closure). Note also that the has.add test for any static value is rewritten. For example,

has.add("my-feature", function(){
  //some 1000-line monster
});

becomes

0 && has.add("my-feature", function(){
  //some 1000-line monster
});

which reduced to nothing.

Thinking about this, I think I need to rewrite with the value given in staticHasFeatures. This would allow...

staticHasFeatures = {
  "isIE":6
};

and

if(has("isIE")<9){
  //do something
}

to be rewritten

if(6<9){
  //do something
}

which closure would reduce to

//do something

comment:6 Changed 8 years ago by ddumont

Yes I agree that could be done. However transporting more than true/false over the wire (since our optimizations are done dynamically) is not as terse as simply true/false. I was hoping the current limitations of your optimizer would lead you to agree with the strict adherence to the intent laid out in the has.js readme about tests returning truthy. While not mandatory to be booleans, there should be no expectation of anything but a truthy interpretation.

comment:7 Changed 8 years ago by ddumont

I also think that if(has("isIE")<9){

do something

}

Is very strange language... I'd really like to see has.js not become a container of named pseudo-globals :-/

comment:8 in reply to:  6 ; Changed 8 years ago by Rawld Gill

Replying to ddumont:

However transporting more than true/false over the wire (since our optimizations are done dynamically) is not as terse as simply true/false.

Hmmm, I'm not clear how

{
  "my-feature":true,
  //etc.
}

is better than

{
  "my-feature":6.0
  //etc.
}

Could you elaborate a little more?

comment:9 in reply to:  8 Changed 8 years ago by ddumont

Replying to rcgill:

Could you elaborate a little more?

http://server.com/combo?modules=...&has=isIE:!isWebkit:!isGecko:dojo-combo-api:!etc&other stuff

Url/cookie space is a premium. We're already considering filtering the has conditions, but still... The potential for misuse is pretty large if you shove a whole novel into a has condition. To me it just doesn't seem like it fits in with the intent of the library, though the author of it I guess would have final say on that matter.

comment:10 Changed 8 years ago by ddumont

Also, since this is dynamic, we are relying on the predefined has conditions specified in the loader(or builder) configs.

Any function that you can has.add() is potentially useful as you discover new capabilities of the browser. I've personally seen code in the wild that does stuff like: has.add('theme', 'MyTheme?') only to use the has condition later to determine what styles to load.

With AMD I've seen an emphasis on avoiding globals as much as possible. To allow this kind of storage seems to be encouraging these "has-scoped" globals. I can certainly imagine situations where this would be convenient, to be sure. I do think, however, that evaluating these tests as 'truthy only' will lead to code that makes a bit more sense when reading it.

comment:11 Changed 8 years ago by ddumont

we are not relying.. Sorry. I can't seem to edit my comments.

comment:12 Changed 8 years ago by Rawld Gill

Status: newassigned

comment:13 Changed 8 years ago by Rawld Gill

Milestone: tbd1.7

comment:14 Changed 8 years ago by Bryan Forbes

I wanted to point out that the original assessment of the has.js library is somewhat incorrect. If you look at the source of has.js, you'll notice several tests returning null or undefined for a "not applicable" case. If you look at the test for "bug-string-split-regexp", if strings in the browser don't contain a split method, obviously the test is not applicable, so the test returns null. Another case is when a test isn't defined, has() will return undefined. This makes the original has() a quad-state return where a falsey value can mean several things in certain contexts, but for most purposes can be skipped.

Dojo's implementation of has() has taken this a bit further and made it so truthy values can mean a couple of different things based on context, but since they are truthy, it means it needs to be handled. I think this is quite acceptable (as one of the people that helped Pete start has.js) and adds information that might otherwise have to be included with a similar test later on.

Let's take a look at the case of CSS styles. If we test to see if a browser supports borderRadius, we're going to have to test all of the vendor prefixes. If we just return "true", we're going to have to test all of the vendor prefixes again to get the right implementation. Why not return the vendor prefix (or the style name) that we've detected from has() and save ourself some bytes and CPU cycles?

comment:15 Changed 8 years ago by Rawld Gill

Milestone: 1.71.8
Owner: changed from Rawld Gill to Kris Zyp
Status: assignednew
Type: defectenhancement

We decided in the 3 August meeting that 1.7 will keep a few non-boolean has values and final resolution of this issue will be moved to 1.8. Further, we will add the disclaimer that that has() is only guaranteed to work for booleans.

This issue was discussed here: http://mail.dojotoolkit.org/pipermail/dojo-contributors/2011-August/025189.html

Lastly, there is the idea that trimming on features is different than trimming on browser versions and that maybe these should handled with a different API in the code.

comment:16 Changed 7 years ago by Colin Snover

Milestone: 1.82.0

1.8 is frozen. Move all enhancements to next release. If you need an exemption from the freeze for this ticket, contact me immediately.

comment:17 Changed 7 years ago by bill

Milestone: 2.01.8
Summary: Prevent unsupported use of has.js feature testingRestrict has("...") to return booleans

From From http://thread.gmane.org/gmane.comp.web.dojo.devel/15477/focus=15481 it looks like Kris wants to close this ticket as wontfix, in which case we might as well close it now, rather than waiting until 2.0.

Last edited 7 years ago by bill (previous) (diff)

comment:18 Changed 7 years ago by Kris Zyp

Resolution: wontfix
Status: newclosed

It seems that the majority are in favor retaining the ability to return different values.

Note: See TracTickets for help on using tickets.