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

Functions should be treated as objects for purposes of updating computations #413

Closed
Rich-Harris opened this issue Mar 27, 2017 · 2 comments
Labels

Comments

@Rich-Harris
Copy link
Member

This example shows the bug (which I did just encounter in a real application, as bizarre as the example looks) — computed values are recomputed if one or more of their dependencies a) are among the changed items AND b) the value has changed from its previous value or is a non-primitive value (since objects and arrays could have been mutated).

That doesn't take account of functions, whose return value might be dependent on a closure. So the check needs to be updated:

Current code:

function applyComputations ( state, newState, oldState, isInitial ) {
  if ( isInitial || ( 'time' in newState && typeof state.time === 'object' || state.time !== oldState.time ) ) {
    state.getFoo = newState.getFoo = template.computed.getFoo( state.time );
  }
	
  if ( isInitial || ( 'getFoo' in newState && typeof state.getFoo === 'object' || state.getFoo !== oldState.getFoo ) ) {
    state.foo = newState.foo = template.computed.foo( state.getFoo );
  }
}

Better code:

function differs ( a, b ) {
  return ( a !== b ) || ( a && typeof a === 'object' ) || ( typeof a === 'function' );
}

function applyComputations ( state, newState, oldState, isInitial ) {
  if ( isInitial || ( 'time' in newState && differs( state.time, oldState.time ) ) ) {
    state.getFoo = newState.getFoo = template.computed.getFoo( state.time );
  }
	
  if ( isInitial || ( 'getFoo' in newState && differs( state.getFoo, oldState.getFoo ) ) ) {
    state.foo = newState.foo = template.computed.foo( state.getFoo );
  }
}
@Conduitry
Copy link
Member

In that example, isn't getFoo arguably no longer a pure function? That's not to say there's not still some bug here, or that there's not some other less insane-looking way to reproduce this.

On the other hand I guess in this case burdening everyone with an extra typeof to handle this case isn't really all that much of a burden.

@Rich-Harris
Copy link
Member Author

or that there's not some other less insane-looking way to reproduce this.

Ha, that's fair. The place I encountered this was in an app that used D3 scales — rather than generating a new function when the dimensions change, the D3 convention is to call a method of a function that updates the values inside the closure where the function is generated:

https://svelte.technology/repl?version=1.13.1&gist=fcdc653576d0ffb156b4d863cd3e5874

If you shrink the width of the output window you'll see that barwidth isn't being recomputed. If you add the width parameter, it starts working — but it should consider xScale to have 'changed' and not need that extra encouragement.

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

No branches or pull requests

2 participants