Add delete method to observable map #24
Conversation
|
The Stencil subscription need to be updated as well. Also, we would need some test cases on The issues I can think of with using 1.- We need to make sure that we can ask for a key before it's been added and still have it updated when some code actually adds it. |
- update stencil subscription to listen for deletes - add example usage in test-app
|
@Serabe I updated the PR with an update to the stencil subscription and an example usage in the
This works fine,
This is an issue. As subscriptions are bound to component lifetimes it doesn't clean up its subscriptions until its no longer connected, so a store with a lot of churn, and a long lived component will result in an ever growing subscription array. I have an idea of how to fix this, will try something out. |
|
With the current example, the solution proposed in the issue will work without any of the concerns of adding delete to store. |
Yep, the workaround is fine, perfectly happy to leave as is and go that route.
I was continuing working on this because it felt it would be nice to have it baked in, but I'm happy to drop it and close the PR and issue. I've pushed a commit that makes the stencil subscription only subscribe to the keys used in an elements most recent render. This should always be enough of a subscription, as conditional rendering based on an item in the store will cause an access (thereby subscribing), and if that condition changes, it is already re-rendering, so will get the latest from the store. Let me know if you want me to clean it up / add tests, otherwise I'm happy to close out with the workaround. |
|
The problem I'm seeing with that approach is that it does not fix all the issues. Let's look into the following component (I'm writing it directly here, without checking any syntax, so some it might contain some errors): @Component({ tag: 'paginated-items' })
export class PaginatedItems {
@Prop() page = 0;
@Prop() itemsPerPage = 10;
get itemsToRender() {
const items = [];
const { page, itemsPerPage } = this;
const limit = (page + 1) * itemsPerPage;
for (let i = page * itemsPerPage; i < limit; i++) {
items.push(dataStore.get(`id-${i}`));
}
return items;
}
render() {
return <Host>{ this.itemsToRender.map(item => <div>{item.name}</div>) }</Host>
}
}If we use it like: <paginated-items page={page} />
<button onClick={() => page--}>Previous</button>
<button onClick={() => page++}>Next</button>All the re-renders to So far, we are increasing the number of listeners without a reliable way of limiting them. If we could find a way to add the delete method without any drawback, it'd be great, but so far I'm seeing added problems (extra subscriptions hard to clean, extra code) without a significant amount of benefits as a counterpart. |
They are cleaned up with the approach in the PR. <paginated-items page={page} itemsPerPage={3} />
<button onClick={() => page--}>Previous</button>
<button onClick={() => page++}>Next</button>(Excuse my pseudo code, hopefully it makes sense) First render:
|
|
Sorry for taking so long, but I need to look deeper into the code and run some tests and those are going to take time. Need to slot some time for it. Sorry! |
|
Hi! In first place, let me apologise for taking so long, but I wanted to make sure I gave a good look at this issue before going on with the talk and I've been having some busy weeks. The I've taken a good look at the code and try to write something with it. The task I tried to accomplish is simple: Have a table with two columns (key and value) and show those same data are displayed in another table (just displayed, not editable). The code is crappy, but was just a proof of concept. It can be found at the end of this comment. The goal could not be achieved as we would need to rerender any component using the store when we add a key. At that point, the perf of doing it at the store level or doing it in a library built on top of it is indistinguishable. import { Component, ComponentInterface, Host, h, State } from '@stencil/core';
import { createStore } from '@stencil/store';
function* generateRandomString() {
while (true) {
const result = [];
for (let i = 0; i < 10; i++) {
result.push(String.fromCharCode(Math.floor(Math.random() * (91 - 65)) + 65));
}
yield result.join('');
}
}
function* filter(gen, predicate) {
let done, value;
do {
let obj = gen.next();
done = obj.done;
value = obj.value;
if (predicate(value)) {
yield value;
}
} while (!done);
}
function* random() {
while (true) {
yield Math.random();
}
}
const values = createStore<Record<string, number>>({});
@Component({
tag: 'spread-sheet',
shadow: true,
})
export class SpreadSheet implements ComponentInterface {
@State() keys = new Set<string>();
componentWillLoad() {
this.fillValues();
}
getKeyGenerator() {
return filter(generateRandomString(), (str) => !(str in values));
}
fillValues() {
const newKeys = new Set<string>();
values.reset();
const keyGen = this.getKeyGenerator();
const randomGen = random();
for (let i = 0; i < 500; i++) {
const { value } = keyGen.next();
newKeys.add(value);
values.set(value, randomGen.next().value || 0);
}
this.keys = newKeys;
}
render() {
const { keys } = this;
console.log('SERABE', keys, values);
return (
<Host>
<button onClick={() => this.fillValues()}>Redo</button>
<table>
<thead>
<tr>
<td>Key</td>
<td>Value</td>
</tr>
</thead>
<tbody>
{Array.from(keys.entries()).map(([key]) => (
<tr>
<td>
<input
value={key}
onChange={(evt) => {
const value = values.get(key);
values.delete(key);
values.set((evt.target as any).value, value);
}}
/>
</td>
<td>
<input
value={values.get(key)}
onChange={(evt) => {
values.set(key, (evt.target as any).value);
}}
/>
</td>
</tr>
))}
</tbody>
</table>
<table>
<thead>
<tr>
<td>Key</td>
<td>Value</td>
</tr>
</thead>
<tbody>
{Array.from(keys.entries()).map(([key]) => (
<tr>
<td>{key}</td>
<td>{values.get(key)}</td>
</tr>
))}
</tbody>
</table>
</Host>
);
}
} |
No worries, sorry for being slow to reply also. Busy couple of weeks.
Kind of, but this is not the goal of the PR. Lets say we adjust your code to look like this: <tbody>
{Array.from(keys), (key) => (
<spread-sheet-row key={key} storeKey={key} />
))}
</tbody>and the
True, but that is a change to However, if we were to modify the data on a single, existing key: values.set(existingKey, randomGen.next().value || 0);In the workaround, we would have to rerender everything, but in the updated version, we only need to update the I'll leave it up to you if that is a worthwhile goal, feel free to close the PR and issue if not. In the end I went for a similar workaround as you initially proposed. It works fine. |
This PR implements the delete method described in #23
Tests, documentation, and examples will be added on go-ahead for #23
fixes: #23