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

this._changeHandlers was undefined when calling cancel() #1178

Merged
merged 3 commits into from
Feb 23, 2018

Conversation

GarethOates
Copy link

This change fixes Issue #1177 where the scope of the cancel() function was not bound correctly and thus when attempting to call cancel() a type error of this._changeHandlers is undefined was created.

@codecov-io
Copy link

codecov-io commented Feb 16, 2018

Codecov Report

Merging #1178 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1178   +/-   ##
=======================================
  Coverage   91.75%   91.75%           
=======================================
  Files         127      127           
  Lines        4572     4572           
  Branches     1495     1495           
=======================================
  Hits         4195     4195           
  Misses        159      159           
  Partials      218      218
Impacted Files Coverage Δ
src/parse/read/directives.ts 73.43% <0%> (ø) ⬆️
src/parse/read/expression.ts 100% <0%> (ø) ⬆️

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 8330f32...957179f. Read the comment docs.

@PaulBGD
Copy link
Member

PaulBGD commented Feb 17, 2018

Could you create a test that fails without this change?

@GarethOates
Copy link
Author

Added a test that fails without this change.

@Conduitry
Copy link
Member

Anyone know what the performance difference is between calling .bind(this) and saving a reference with var self = this; outside the function and using self... instead of this...?

@Rich-Harris
Copy link
Member

Thanks @GarethOates!

Anyone know what the performance difference is

I put together a lil ESBench that suggests saving a reference is cheaper, which I suppose makes sense (maybe the difference is less noticeable if you're calling the bound function many times, but cancel() is only called once) — I tweaked the PR accordingly in 957179f.

@Rich-Harris Rich-Harris merged commit fba8a94 into sveltejs:master Feb 23, 2018
@GarethOates GarethOates deleted the storeCancelScope branch February 23, 2018 14:53
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.

5 participants