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

Fix substitution evaluation (fixes #143) #144

Merged
merged 1 commit into from
May 26, 2018

Conversation

fatfisz
Copy link
Collaborator

@fatfisz fatfisz commented May 3, 2018

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).

@fatfisz fatfisz requested a review from zspecza May 3, 2018 22:52
@codecov
Copy link

codecov bot commented May 3, 2018

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%> (ø) ⬆️

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
Copy link

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

@fatfisz
Copy link
Collaborator Author

fatfisz commented May 25, 2018

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
Copy link

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 fatfisz merged commit 769aea0 into master May 26, 2018
@fatfisz fatfisz deleted the fix-substitution-evaluation branch May 26, 2018 13:26
@fatfisz
Copy link
Collaborator Author

fatfisz commented May 26, 2018

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

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

Successfully merging this pull request may close these issues.

2 participants