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

add updating guard to binding callback #5126

Merged
merged 2 commits into from
Jul 17, 2020

Conversation

tanhauhau
Copy link
Member

@tanhauhau tanhauhau commented Jul 10, 2020

Fixes #3180
Fixes #5117

binding callback should be called only when changes happen internally.

similar with <input bind:value>, the on input only call when typing into the input, but not when programmatically set the input.value.

a brief description of the bug:

  • we have a <Component bind:value /> and when we $$invalidate(value) from outside,
  • component.$set({ value }) which internally calls $$invalidate(value_internal)
  • which then calls the binding callback $$.bound[value]()
  • in the binding callback, we call the $$invalidate(value)
  • which again calls the component.$set({ value }) which internally calls $$invalidate(value_internal)
  • only this time round, the !safe_not_equal(value, value_internal), so the binding callback, $$.bound[value](), did not get called.

as you can see from the sequence of steps, $$invalidate(value) get called twice.

Also, it seemed like it will add quite some bytes into the compiled code, should we rename the variable to a shorter name?
or use 1 / 0 instead of true / false?

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases, features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

  • Run the tests with npm test or yarn test)

@Conduitry Conduitry marked this pull request as draft July 13, 2020 14:07
@Conduitry
Copy link
Member

The test failure here appears to be legitimate. I'm marking this as Draft for now.

@tanhauhau tanhauhau force-pushed the tanhauhau/gh-3180 branch from 0a76f66 to a7985a8 Compare July 14, 2020 04:17
@tanhauhau tanhauhau marked this pull request as ready for review July 14, 2020 04:18
@tanhauhau tanhauhau force-pushed the tanhauhau/gh-3180 branch from a7985a8 to 0b8cdf8 Compare July 14, 2020 04:26
@Conduitry
Copy link
Member

Hrm. I'm not thrilled about the per-component byte cost that this adds. I don't have a better suggestion right now.

@Conduitry
Copy link
Member

What if we rework this a bit so that $set() is defined on the class itself, where it sets and unsets the guard flag, and calls something like this.$$set() or whatever, and it's that function that each instance overrides? Still a slight increase in cost, but less of a per-component one than this.

Actually, if we're doing that, maybe it makes sense to redo the change in #5123 slightly so that the is_empty check happens in this new shared $set() method.

@tanhauhau tanhauhau force-pushed the tanhauhau/gh-3180 branch from a64345f to 67e9509 Compare July 15, 2020 09:15
@Conduitry Conduitry merged commit ec0f79c into sveltejs:master Jul 17, 2020
@tanhauhau tanhauhau deleted the tanhauhau/gh-3180 branch July 18, 2020 04:24
hontas added a commit to hontas/svelte that referenced this pull request Jul 18, 2020
* upstream/master:
  add updating guard to binding callback (sveltejs#5126)
  Bump lodash from 4.17.15 to 4.17.19 (sveltejs#5152)
  Bump lodash from 4.17.15 to 4.17.19 in /site (sveltejs#5155)
  Fixes sveltejs#5153 (sveltejs#5154)
hontas added a commit to hontas/svelte that referenced this pull request Jul 18, 2020
* master: (67 commits)
  add updating guard to binding callback (sveltejs#5126)
  Bump lodash from 4.17.15 to 4.17.19 (sveltejs#5152)
  Bump lodash from 4.17.15 to 4.17.19 in /site (sveltejs#5155)
  Fixes sveltejs#5153 (sveltejs#5154)
  invalidate $$props and $$restProps only when there are changes (sveltejs#5123)
  site: use https in link in blog (sveltejs#5148)
  Simplify each block bindings example (sveltejs#5094)
  fix $$props reactive for slots (sveltejs#5125)
  site: add FAQ entry for how to document a svelte component (sveltejs#5131)
  site: remove an obsolete TODO in blog post (sveltejs#5135)
  Increase timeout for unit build
  Increase timeout for unit tests
  -> v3.24.0
  spread condition for input element (sveltejs#5004)
  update changelog
  fix(5018): compare wholeText instead of data (sveltejs#5028)
  html anchor in head (sveltejs#5071)
  error on expression scope store (sveltejs#5079)
  update changelog
  preprocess self-closing script and style tags (sveltejs#5082)
  ...
Truffula pushed a commit to Truffula/svelte that referenced this pull request Aug 3, 2020
taylorzane pushed a commit to taylorzane/svelte that referenced this pull request Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants