Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

undefined/null values in interpolated expressions should return "" #1406

Closed
ricardobeat opened this issue Jun 1, 2011 · 25 comments
Closed

undefined/null values in interpolated expressions should return "" #1406

ricardobeat opened this issue Jun 1, 2011 · 25 comments

Comments

@ricardobeat
Copy link
Contributor

@ricardobeat ricardobeat commented Jun 1, 2011

On most templating/string formatting implementations, falsy values result in an empty string:

"#{something} here"
=> " here"

whereas in CoffeeScript you get the .toString() representation:

something = x if y // condition fails
"#{something} here"
=> "undefined here"
something = null
"#{something}"
=> "null"

You can say this matches expectations, coming from js, but it could be better. I have yet to see a case where you actually want to print "undefined", "false" or "null" to a string, so returning an empty string would break expectations in a good way :) If you really want it .toString() and typeof will still be there for you.

As a result, this:

console.log "#{not(i%3) and 'fizz' or ''}#{not(i%5) and 'buzz' or ''}" or i for i in [1..100]

could be written as

console.log "#{'fizz' if !(i%3)}#{'buzz' if !(i%5)}" or i for i in [1..100]

That would at principle mean wrapping interpolated expressions in (expression || ''). That wouldn't work for 0 though. Special delimiters?

@satyr
Copy link
Collaborator

@satyr satyr commented Jun 1, 2011

$ goruby -e 'p "#{false}"'
""
@ricardobeat
Copy link
Contributor Author

@ricardobeat ricardobeat commented Jun 1, 2011

Removed the reference to Ruby, but my IRB still prints "false" :(

@michaelficarra
Copy link
Collaborator

@michaelficarra michaelficarra commented Jun 1, 2011

could be written as

console.log "#{'fizz' unless i % 3}#{'buzz' unless i % 5}" or i for i in [1..100]

Fixed that for you. At first glance, I really like this idea. People have complained about this behaviour before, but I never thought of fixing it. This is inconsistent, but it truly appears to be inconsistent in a good way as you put it. This proposal receives a pleasantly surprised (but still a little hesitant) +1 from me.

@ricardobeat: Can we get some suggested compilations?

@ricardobeat
Copy link
Contributor Author

@ricardobeat ricardobeat commented Jun 1, 2011

Thanks. Well, thinking of it now, it's easy for the simple optimized cases, simply returning "" instead of void 0:

> console.log "#{"fizz" unless i%3}"
console.log("" + (!(i%3) ? "fizz" : "")

> console.log "#{if x then y}"
console.log("" + (x ? y : ""))

For complex cases a temporary variable can be used:

> console.log "#{if x then y else z}" // can't optimize this
var _ref; console.log("" + ((_ref = x ? y : z) == null ? "" : _ref))

> console.log "#{foo(bar)}"
var _ref; console.log("" + ((_ref = foo(bar)) == null ? "" : _ref))

The other alternative I can think of right now is a predefined function:

__string = function(val){
    if (val == null) return "";
    return val;
}

> "#{complex_expression}"
"" + __string(complex_expression)

Adds less pollution to the output but breaks the "no new objects/functions" rule.

I know nothing about the lexing/parsing/compiling rules so I have no idea if this might be feasible, just thought I'd put it out.

@michaelficarra
Copy link
Collaborator

@michaelficarra michaelficarra commented Jun 1, 2011

Some optimizations for your first two examples:

> console.log "#{"fizz" unless i%3}"
console.log(i%3 ? "" : "fizz");

> console.log "#{if x then y}"
console.log(x ? ""+y : "");

You make comparisons to null in these next examples. I thought we were testing for truthiness. Also, _refs are unnecessary in the cases you give.

> console.log "#{if x then y else z}" // can't optimize this
console.log("" + ((x ? y : z) || ""));

> console.log "#{foo(bar)}"
console.log("" + (foo(bar) || ""));

Is there a case that needs a temporary variable? @ricardobeat: have anything more edge-casey?

It looks like we can just apply this pattern for any X:

> console.log "#{X}"
console.log("" + (X || ""));

... which is a good thing. The top two optimizations are just simplifications of this applied pattern.

edit: It looks like null tests are intended, so ignore all of my example compilations above. See @ricardobeat's suggestion directly below mine.

@ricardobeat
Copy link
Contributor Author

@ricardobeat ricardobeat commented Jun 1, 2011

Another option:

"" + [0].join("") => "0"
"" + [null].join("") => ""
"" + [undefined].join("") => ""
"" + [complex_expression].join("") // no added complexity

one more reason to switch to array join? then we'd get this for free.

"#{x} is like #{y} but not like #{foo(bar)}"
=> [x, " is like ", y, " but not like ", foo(bar)].join("")
@ricardobeat
Copy link
Contributor Author

@ricardobeat ricardobeat commented Jun 1, 2011

@michaelficarra I took the first examples from coffee's compiled output.

The problem is 0 is falsy. Consider printing a loop index:

for i in [0..3]
    console.log "item #{i}"
>  
> 1
> 2
> 3

That would be "badly" unexpected.

null == undefined, that's what we need to test to keep zeroes intact. false will still fall through. edit: that might make sense since true will always be printed.

michaelficarra added a commit that referenced this issue Jun 1, 2011
@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Jun 1, 2011

Before closing this ticket, can someone whip up a JSPerf that demonstrates the perf-hit isn't too bad from switching to join? I remember that we evaluated both alternatives when implementing interpolation in the first place.

@TrevorBurnham
Copy link
Collaborator

@TrevorBurnham TrevorBurnham commented Jun 1, 2011

Looks like + is significantly faster (tested in Chrome, Safari and Firefox... JSPerf seems to be having problems displaying recorded results right now):
http://jsperf.com/join-vs-plus-concatenation

Also, the + output is more readable. OTOH, goodness knows I'm sick of writing "You did #{superlative ? ''} well!" And I can't imagine a small performance hit is going to push people away from using interpolations (if you're doing a lot of them, you should probably be using a buffer rather than a string anyway). So I'm neutral on this proposal.

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Jun 1, 2011

Given the perf hit (2x slower in Safari, 4x slower in Chrome), I think we should find a few more use cases before deciding this one. I actually can't think of many situations where you wouldn't want to print "undefined" in an interpolation. Wouldn't you expect it to be the string coercion of every interpolated value? Wouldn't you want to see the error instead of failing silently? Don't you need to print the string differently anyway if the value is missing?

Use-cases would help clear up those concerns.

@ricardobeat
Copy link
Contributor Author

@ricardobeat ricardobeat commented Jun 1, 2011

Here you go: http://jsperf.com/js-string-concatenation-vs-array-join

Array.join() comes with a huge performance penalty on V8. On TraceMonkey it's the opposite, join is 25% faster than concatenation.

Surprise: concatenating expressions checking for null/undefined values is faster than concatenating simple variables #mindfucked (on browsers, in node it's ~10% slower)

@michaelficarra
Copy link
Collaborator

@michaelficarra michaelficarra commented Jun 1, 2011

I edited Trevor's JSPerf to show that we can make a little three-statement interpolation helper that behaves close to concatenation speed (in chome, at least). That'll be the best of both worlds, I believe.

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Jun 1, 2011

Nope -- we definitely don't want to introduce a helper for this.

@michaelficarra
Copy link
Collaborator

@michaelficarra michaelficarra commented Jun 1, 2011

@jashkenas: Then I guess it's down to either rejecting this proposal (probably not a good idea) or compiling to the equivalent of (X ? '') for each interpolated value X. I'm assuming that the [].join("") method was no longer an option because of the pretty substantial performance impact.

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Jun 1, 2011

I think that the array join method is acceptable -- it's still millions of interpolations / sec -- if the utility in real-world code can be demonstrated with use cases. In this case:

"There are #{count} documents."

"There are  documents"
"There are undefined documents"

How is the former better? Usually, if I accidentally interpolate an undefined value, I'd rather see the error.

michaelficarra added a commit that referenced this issue Jun 1, 2011
concatenation with existence checks (`X ? ""` for any `X`). Faster but
less readable than the other `[].join("")` method. Tests to come.
@michaelficarra
Copy link
Collaborator

@michaelficarra michaelficarra commented Jun 1, 2011

@jashkenas: I'd say the major use case is the inline conditionals as mentioned in the original post. @TrevorBurnham: You said this was a common pattern for you, right?

On a side note, the concatenation-style compilation could be so much nicer if JS just had the binary existential operator... @DmitrySoshnikov: what's the status of the ?? "default" operator proposal?

@ricardobeat
Copy link
Contributor Author

@ricardobeat ricardobeat commented Jun 2, 2011

if I accidentally interpolate an undefined value, I'd rather see the error

In the other hand, you'd prefer to hide it from users. No one likes a scary and confusing "The undefined has been added" message. "The has been added." still makes it clear that there is an error while being more friendly.

It also matches the logic behind the stringifying in Array::join(), which is kind of an interpolation. I'm getting too esoteric, but it makes sense to me that when you're working with a string nothing means "" instead of the strings "null" or "undefined".

"I have #{x} turtle#{'s' if x > 1}"
"Active services: #{s1} #{s2} #{s3}" // a dumber but simpler way to do [s1, s2, s3].join(" ")

I mostly like it for the simpler inline conditionals; as @michaelficarra said it's the major use case.

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Jun 2, 2011

So, if the use-cases are:

"the  has been added"
"I have  turtle"
"Active services:      "

Then this is really a bug, not a feature. @michaelficarra -- please revert this on master, and stick it on a branch, if you like.

@michaelficarra
Copy link
Collaborator

@michaelficarra michaelficarra commented Jun 2, 2011

@jashkenas: I never pushed it to master. There are two implementations on branches issue1406 and issue1406_alt. Though how can you argue that it's not a common case to have to use "#{X ? ""}" in your interpolations when just last night I used it in my fix for #1409? And others have said that they often need to do that as well.

edit: I have deleted branches issue1406 and issue1406_alt. The commits that they contained were e5f4ccd and 2e0af23 respectively.

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Jun 2, 2011

Ah, glad to hear it. That's why I'm looking for real use cases. There are certainly plenty in the string-building portions of the compiler.

@ricardobeat
Copy link
Contributor Author

@ricardobeat ricardobeat commented Jun 2, 2011

@jashkenas on the same note, I don't see how

"I have undefined turtleundefined"
"the undefined has been added"

is a feature. x is a number in the first example, not a null value.

@satyr
Copy link
Collaborator

@satyr satyr commented Jun 3, 2011

Can't you just use "#{[X]}" as a tighter (but slightly heavier) version of "#{X ? ''}"?

console.log "#{['Fizz' unless i%3]}#{['Buzz' unless i%5]}" or i for i in [1..100]
@ricardobeat
Copy link
Contributor Author

@ricardobeat ricardobeat commented Jun 4, 2011

Can't you just use "#{[X]}" ?

jaw dropped

I think this pattern resolves the issue: it's opt-in and doesn't have a performance hit for other strings.

@TrevorBurnham
Copy link
Collaborator

@TrevorBurnham TrevorBurnham commented Jun 7, 2011

Excellent. So, sounds like we can close this?

@ricardobeat
Copy link
Contributor Author

@ricardobeat ricardobeat commented Jun 9, 2011

closing we are. thanks for all the fish.

(arguments for [].join remain, but it's probably best to continue that discussion elsewhere)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants