-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Allow #each
block to handle arbitrary non array like iterables
#7425
Comments
Rich Harris has recently talked about supporting this too: https://youtu.be/dB_YjuAMH3o?t=4m18s |
Would love to see this added to the roadmap for v4 |
@joeally Just confirming— once implemented, this wouldn't even require using I'd think this could be as simple as— <script>
const myMap = new Map();
myMap.set('key1', 'value1');
myMap.set('key2', 'value2');
myMap.set('key3', 'value3');
</script>
{#each myMap as [k, v]}
<h1>Key: {k}, value: {v}</h1>
{/each} |
@brandonmcconnell @joeally What Brandon has provided is the API I would love to see. IMO Svelte should obfuscate the implementation details so the template reads the same whether you're iterating over a map or an array. |
What will likely happen is that the each block calls |
Yeah, we need to buffer the whole iterable into a single array anyway to implement the keyed-each minimal edit stuff, so we might as well just use |
Doesn't that defeat the performance benefit of Set and Map, though, when dealing with massive data sets? In native JS, we can iterate over sets and maps using for..of and retain those perf benefits without needing to port their contents to an array. It would be ideal to keep those benefits. const myMap = new Map();
myMap.set('key1', 'value1');
myMap.set('key2', 'value2');
myMap.set('key3', 'value3'); I ran some benchmarks just to demonstrate this difference, and Map iteration is faster across all (by over 50%):
The fact alone that taking advantage of the perf benefits is so attainable and not something other frameworks have done yet is a huge opportunity for Svelte, and I think we'd be falling short to not take a key stake in big (and attainable) performance wins like this. I think |
The DOM is slow, and I am almost certain that losing the ability to do minimal DOM updates would far outweigh what we'd gain by only iterating through the array once with |
I agree that a solution like What "ability to do minimal DOM updates" would be lost by switching to use If we use a lightweight abstraction like the Implementation— see on TS Playground function isIterable<T>(obj: Iterable<T> | ArrayLike<T>): obj is Iterable<T> {
return typeof (obj as Iterable<T>)[Symbol.iterator] === 'function';
}
function iterateOver<T>(obj: Iterable<T> | ArrayLike<T>, expose: Function) {
if (isIterable(obj) && !Array.isArray(obj)) {
/* non-array iterables (array iteration using a traditional `for` loop is more performant) */
let i = 0;
for (const elem of obj) {
expose(elem, i++);
}
} else {
/* arrays and array-likes */
const indexedObj: ArrayLike<T> = (
'length' in obj && typeof obj.length === 'number'
? (obj as { length: number })
: Array.from(obj)
);
for (let i = 0; i < indexedObj.length ?? 0; i += 1) {
expose(indexedObj[i], i);
}
}
} In ☝🏼 this example, I am using an imaginary "expose" function, but that would just be replaced by whatever logic or helper function you use to slot in the HTML for each Notes on why I used the |
And while it arguably reads easier with the function iterateOver<T>(obj: Iterable<T> | ArrayLike<T>, expose: Function) {
if (typeof (obj as Iterable<T>)[Symbol.iterator] === 'function' && !Array.isArray(obj)) {
/* non-array iterables (array iteration using a traditional `for` loop is more performant) */
let i = 0;
for (const elem of (obj as Iterable<T>)) {
expose(elem, i++);
}
} else {
/* arrays and array-likes */
const indexedObj: ArrayLike<T> = (
'length' in obj && typeof obj.length === 'number'
? (obj as { length: number })
: Array.from(obj)
);
for (let i = 0; i < indexedObj.length ?? 0; i += 1) {
expose(indexedObj[i], i);
}
}
} — see on TS Playground Here's a JSDoc version 👇🏼/**
* Iterates over an iterable object or array-like object and exposes each element to a callback function.
* @param {Iterable<T> | ArrayLike<T>} obj - The iterable or array-like object to iterate over.
* @param {Function} expose - The callback function to call for each element.
* @returns {void}
* @template T
*/
function iterateOver(obj, expose) {
if (typeof obj[Symbol.iterator] === 'function' && !Array.isArray(obj)) {
/** @type {Iterable<T>} */
const iterableObj = obj;
/* non-array iterables (array iteration using a traditional `for` loop is more performant) */
let i = 0;
for (const elem of iterableObj) {
expose(elem, i++);
}
} else {
/** @type {ArrayLike<T>} */
const indexedObj = (
'length' in obj && typeof obj.length === 'number'
? obj
: Array.from(obj)
);
/* arrays and array-likes */
for (let i = 0; i < (indexedObj.length ?? 0); i += 1) {
expose(indexedObj[i], i);
}
}
} |
@brandonmcconnell -
Yes you're right. I didn't know that |
@joeally Exactly. The Map and Set are not special cases. Every iterable can be iterated over using the same Array types are iterable in the same way that Map, Set, and generators are, but they actually iterate more performantly if iterated over using a plain We can loop over |
I noticed another issue which possibly is similar to this one, #4289 . |
i cant overstate how exciting this is. this is the one problem that forces me to patch svelte at the moment. basically i have an arraylike js proxy object that unfortunately returns typeof as "function" for unrelated reasons at the moment the each statement in svelte checks if something is an object which is not true and its also not clear why only objects with a length attribute should be allowed, instead arg?.length seems like reasonable check for array like. currently the check is
additionally supporting generic iterables would solve a big pain for me as i often iterate of entries where the number is not known before the iteration ends, this forces me to guess the length and then run into all kinds of issues when when its off |
closes #7425 Uses a new ensure_array_like function to use Array.from in case the variable doesn't have a length property ('length' in 'some string' fails, therefore obj?.length). This ensures other places can stay unmodified. Using for (const x of y) constructs would require large changes across the each block code where it's uncertain that it would work for all cases since the array length is needed in various places.
closes #7425 Uses a new ensure_array_like function to use Array.from in case the variable doesn't have a length property ('length' in 'some string' fails, therefore obj?.length). This ensures other places can stay unmodified. Using for (const x of y) constructs would require large changes across the each block code where it's uncertain that it would work for all cases since the array length is needed in various places.
Amazing to see this change in Svelte 4.0. Thanks for all of the hard work @dummdidumm and anyone else who worked on it. |
@dummdidumm Thanks for all your work on this! Did you see my earlier comments about iterating over iterable values directly (maps, sets, generators, etc.), which greatly improves performance? If I'm reading the merged PR accurately, it looks like those would all now be converted to arrays using |
That's correct. We use |
@dummdidumm by that argument you could just say people should wrap iterables with array.from in application layer, that way there would be 0 lines of code change in svelte and we would not break expectations. supporting iterables means by definition allowing to start rendering pages of entries from the iterable without accessing length, if this is too much work at the moment this should just be postponed but this will lead to many issues |
Also, for many iterables, like maps and sets, you can use The helper function I outlined above is a single function that accounts for this and manages all cases. With a small tweak, you could limit the performance boost to maps and sets for their Were there any other downsides to using that? As @lucidNTR mentioned, users can also do that conversion to arrays explicitly if it's what they want, but in most cases, I explicitly work with maps and sets because they're generally more performant with large datasets. The current implementation flips that upside down and is slower than using an array to begin with. |
The docs state how the iterable is iterated and what expectations there are to it, namely that they are static and synchronous and are iterated over eagerly. You won't get some kind of intermediate rendering support from that (like, you can't iterate five, then another five two seconds later) - this is due to how the rendering system works. Things like The code snippet with the iteration over either a regular array or the iterable can't just be applied, it's not that simple. As I said, the length is needed in various places, like when you have keyed each blocks with transitions on the items. The performance argument is weak because if you're using iterables you're already slower compared to plain old arrays, so if you're performance aware you shouldn't use them anyway. Yes, in summary this means "just" a small QOL improvement which is that you don't need to do |
Implemented in #8626 |
Not sure why I'm not seeing @eabald's comment here anymore, so reposting it here. As it echos the same comments many of us have voiced before:
I think maps and sets both really need this. This is a massive performance deficit for Svelte and can easily be avoided as demonstrated earlier and still support an index, so I'd also argue that the lack of a natural iterable index is a weak argument as well. |
@brandonmcconnell I'm pretty sure that I'm not the person You wanted to tag in this comment. My knowledge about Svelte is limited to knowing what it is, so I doubt that I made this rather constructive comment... |
@eabald My sincere apologies — I got an email notification from someone of the same name and thought it was you. |
No problem :)
|
Describe the problem
Consider the following REPL:
Obviously one could trivially achieve this by wrapping myMap in
Array.from
or spreading into an array but it would be nice if svelte could handle iterables in addition toArrayLike
objects.Describe the proposed solution
The problem is caused by the fact that svelte generates the code which loops over the each value using its
.length
property. This can be seen below:and
As you can see it assumes that
each_value
(in this casemyMap
) has alength
property. It seems to me that we could easily generate the following code which is equivalent:With
get_each_context
rewritten to be:It's worth noting that the
for...of
syntax isn't supported by IE11 but the svelte compiler appears to recommend the use of the spread operator which also isn't supported in IE11 so perhaps this isn't an issue.Alternatives considered
The alternative here is to leave it as it is and tell people to use
Array.from
and or[...mySet]
. It isn't the end of the world.But given that it seems so trivial to support arbitrary iterables and that javascript can loop over iterables I can't see why svelte wouldn't implement this. Is it because you want any infinite loops to be in user code rather than generated code?
Importance
nice to have
The text was updated successfully, but these errors were encountered: