zspecza / common-tags

🔖 Useful template literal tags for dealing with strings in ES2015+
Other
1.99k stars 60 forks source link

Fix substitution evaluation (fixes #143) #144

Closed fatfisz closed 6 years ago

fatfisz commented 6 years ago

From the spec:

The string conversion semantics applied to the Expression value are like String.prototype.concat rather than the + operator.

Currently method processSubstitutions of TemplateTag is concatenating strings using the + operator, which results in the default hint while calling the abstract operation ToPrimitive. That causes valueOf to be called before toString if the value is an object. This is not in line with the spec and caused confusion (#143).

The fix is to simply use ''.concat (or call String on everything).

codecov[bot] commented 6 years ago

Codecov Report

Merging #144 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #144   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          24     24           
  Lines         102    102           
  Branches       29     29           
=====================================
  Hits          102    102
Impacted Files Coverage Δ
src/TemplateTag/TemplateTag.js 100% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 936d99e...1b9a368. Read the comment docs.

inquisitivecrystal commented 6 years ago

Any word on this? Having to manually call toString is pretty annoying.

fatfisz commented 6 years ago

I'll see what I can do after work today. It seems that @declandewet was unavailable for review (as you may have heard he is recovering from an accident).

inquisitivecrystal commented 6 years ago

I hadn't heard, am very sorry to hear that, and wish him a speedy recovery. In light of his unavailability, could you merge it on your own authority, as a collaborator?

fatfisz commented 6 years ago

I'll add one more thing before releasing the next version. Please give it at most 2 more days and it'll be published!