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 support to computed and store for immutable structures #1164

Merged
merged 6 commits into from
Feb 11, 2018

Conversation

jacwright
Copy link
Contributor

Adds optional performance support for apps using an immutable data structure such as redux. Adds the immutable boolean option for compile and an immutable option to store as well. When these options are used, computed will not recompute if the object has not changed. If your data structure is not immutable you should not use this as svelte cannot know if a mutation was made on objects.

This PR also adds support for Dates and NaN values so computed properties will not recompute if a date has not changed or a value did not change from NaN.

This closes out these issues:

This is my first PR for Svelte. Any feedback would be appreciated!

Adds optional performance support for apps using an immutable data structure such as redux. Adds the `immutable` boolean option for compile and an `immutable` option to store as well. When these options are used, computed will not recompute if the object has not changed. If your data structure is not immutable you should not use this as svelte cannot know if a mutation was made on objects.

This PR also adds support for Dates and NaN values so computed properties will not recompute if a date has not changed or a value did not change from NaN.

This closes out these issues:
* sveltejs#1146
* sveltejs#1161

This is my first PR for Svelte. Any feedback would be appreciated!
@codecov-io
Copy link

codecov-io commented Feb 9, 2018

Codecov Report

Merging #1164 into master will increase coverage by 0.21%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1164      +/-   ##
==========================================
+ Coverage   91.51%   91.72%   +0.21%     
==========================================
  Files         126      127       +1     
  Lines        4526     4569      +43     
  Branches     1462     1491      +29     
==========================================
+ Hits         4142     4191      +49     
+ Misses        163      159       -4     
+ Partials      221      219       -2
Impacted Files Coverage Δ
src/validate/js/propValidators/index.ts 100% <ø> (ø) ⬆️
src/validate/js/propValidators/immutable.ts 0% <0%> (ø)
src/generators/Generator.ts 94.17% <100%> (+0.02%) ⬆️
src/generators/dom/index.ts 96.25% <100%> (+0.21%) ⬆️
...tors/server-side-rendering/visitors/MustacheTag.ts 100% <0%> (ø) ⬆️
.../generators/server-side-rendering/visitors/Text.ts 100% <0%> (ø) ⬆️
src/utils/stringify.ts 100% <0%> (ø) ⬆️
...rators/server-side-rendering/visitors/Component.ts 98.21% <0%> (+0.03%) ⬆️
... and 4 more

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 0ef8229...cb446bc. Read the comment docs.

@Rich-Harris
Copy link
Member

Thank you! This is going to be great.

I'm not sure we can give dates special treatment, since they're mutable objects:

methods: {
  forward() {
    const { date } = this.get();
    date.setHours(date.getHours() + 1);
    this.set({ date });
  }
}

In that situation we can't rely on valueOf.

We could accommodate NaN like so:

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

(i.e. the current differs, but with a != a ? b == b : ...) Given that, maybe differsImmutable could just be this?

function differsImmutable(a, b) {
  return a != a ? b == b : a !== b;
}

Am wondering if we should use the same this._differs trick you've used with Store, so that we don't need to maintain set and setImmutable separately (since otherwise we really ought to have setDevImmutable...)? A nice side-effect of that would be that you could even customise it on a component-by-component basis (e.g. you need to micro-optimise one specific thing and can't opt in to immutable everywhere, perhaps).

@jacwright
Copy link
Contributor Author

Yes, the dates cannot be checked this way, you are right. I've made the changes and also added some tests.

jacwright added a commit to jacwright/svelte-cli that referenced this pull request Feb 10, 2018
This supports this sveltejs/svelte#1164 PR for the command line.
@jacwright
Copy link
Contributor Author

jacwright commented Feb 10, 2018

I'm reworking this some more in order to support customizing on a component-by-component basis. I believe this would remove the need for a compiler option entirely.

Right now I have it to the point where you can pass in immutable to the constructor options like this:

var app = new App({
  target: document.body,
  immutable: true
});

But I feel it would be useful to also add it as a component property like:

export default {
  immutable: false, // or true
  data: () => {
    return {}
  }
};

Thoughts on adding it to the definition like this? Should it be a method like with store?

This allows components to opt in (or out) of using immutable data checking for greater flexibility in app design. It also removes the compiler option.
@jacwright
Copy link
Contributor Author

jacwright commented Feb 10, 2018

The API in the previous comment has now been implemented. There is no compiler option (which means we can close sveltejs/svelte-cli#31 assuming this is an accepted solution). I use the same option flag (immutable) in components and in store. When added to the root it will apply to all subcomponents. A subcomponent set whether it is immutable or not overriding the root option.

This allows individual components to use immutable when the rest of the app is not, useful for expensive computed property calculations. It also allows individual components to NOT use immutable when the rest of the app does, useful for interacting with 3rd-party libraries that might have mutable data.

Future improvements might include setting immutable in the options of component.observe, but I'm not sure how useful this will be and it will change the code more than I am willing to do without more feedback. Perhaps we can come back to that later.

@jacwright
Copy link
Contributor Author

I'm unsure what the failing check is about. Does it just need to be rerun?

@Rich-Harris
Copy link
Member

The failing test is just because codecov has detected that too few of the new lines of code are covered by tests — maybe we need to add another one or two?

Making immutable a runtime option does have a downside — it means that every app needs both differs and differsImmutable to be included because we can't know in advance which one is going to be used. In other words, someone upgrading Svelte after this change would find that their app was larger, even though they weren't taking advantage of the new feature. What if we kept the option on the component definition, but went back to immutable being a compiler option rather than an initialisation option?

Also: not something that this PR needs to consider, but something that occurred to me: deep two-way bindings on components with immutable: true will break, because this...

<input bind:value=foo.bar>

...is basically equivalent to this:

var state = this.get();
state.foo.bar = input.value;
component.set({ foo: state.foo }); // won't trigger an update, because it's the same object

I think we'll probably have to just disallow those sorts of bindings if immutable is true.

This will keep existing code smaller and _mostly_ only add size when using the `immutable` compiler option.
@Rich-Harris Rich-Harris merged commit cb446bc into sveltejs:master Feb 11, 2018
@Rich-Harris
Copy link
Member

Thank you! Merged with a couple of small changes — I put the _differs method on the prototype, since we don't really need to assign it to each component separately, and I removed the runtime option, since a) it means we can generate less code, and b) we're going to need to know at compile time whether a component expects immutable data, if we're to deal with that tricky two-way binding issue I mentioned earlier.

@jacwright
Copy link
Contributor Author

If we can fix the 2-way binding with the immutable flag that would be amazing.

@jacwright jacwright deleted the immutable-support branch February 12, 2018 17:29
@jacwright
Copy link
Contributor Author

Also, I have a CLI PR to add the immutable option sveltejs/svelte-cli#31

@jacwright
Copy link
Contributor Author

jacwright commented Mar 10, 2018

Quick docs

Jotting down some documentation on how this works because I already forgot and had to look through the code to remind myself. 😊 Should probably add this to the website docs I suppose.

Svelte works with mutable data and immutable data by default. In addition to the default behavior Svelte can provide performance gains if you are working with immutable data by more easily knowing when values changed and computed properties need to be recalculated.

To take advantage of these performance gains, add the compiler option { immutable: true } to your Svelte config. Once added, you can instruct all your components to use efficient immutable comparisons by setting immutable to true when creating your root component like this:

const app = new App({
  target: document.body,
  immutable: true
});

To illustrate the different between immutable comparison and mutable comparison consider the following:

<!-- App.html -->
<div>{{ name }}</div>
<script>
export default {
  computed: {
    name: person => {
      console.log('ran computed name');
      return person.firstName + ' ' + person.lastName;
    }
  }
}
</script>
// index.js
const person = { firstName: 'John', lastName: 'Smith' };

const immutableApp = new App({
  target: document.body,
  immutable: true,
  data: { person }
});

immutableApp.set({ person });
immutableApp.set({ person });
immutableApp.set({ person });
// Result's in console:
// > ran computed name


const mutableApp = new App({
  target: document.body,
  immutable: false,
  data: { person }
});

mutableApp.set({ person });
mutableApp.set({ person });
mutableApp.set({ person });
// Result's in console:
// > ran computed name
// > ran computed name
// > ran computed name
// > ran computed name

With immutable turned on, if the object does not change, Svelte assumes the properties did not change either. With immutable turned off, Svelte must always rerun computations in case the object properties changed.

If you have some data that is not immutable, you can instruct a particular component to not use the immutable comparison by defining it with the immutable flag set to false.

<div>{{ name }}</div>
<script>
export default {
  immutable: false,
  computed: {
    name(mutableInput) {
      return mutableInput.firstName + ' ' + mutableInput.lastName;
    }
  }
};
</script>

You may also only want to make a single component compare data with the immutable comparison if most of your data is mutable but you have an expensive computation that you can use immutable data with. In this case, do not pass immutable to the root component as an option and set immutable to true only in the component that will use it.

<div>{{ name }}</div>
<script>
export default {
  immutable: true,
  computed: {
    name(immutableInput) {
      return expensiveCalculation(immutableInput);
    }
  }
};
</script>

You can also tell the Svelte Store whether the data stored there is immutable or not.

import { Store } from 'svelte/store.js';

const store = new Store({
  name: 'world'
}, { immutable: true });

const app = new App({
  target: document.body,
  immutable: true,
  store
});

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.

3 participants