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

#each over iterables #4289

Closed
jmakeig opened this issue Jan 20, 2020 · 21 comments
Closed

#each over iterables #4289

jmakeig opened this issue Jan 20, 2020 · 21 comments

Comments

@jmakeig
Copy link

jmakeig commented Jan 20, 2020

Is your feature request related to a problem? Please describe.

Today, {#each} only works over Array and Array-like objects (i.e. that have a numeric length property). However, ES2015 introduces iterables using the built-in Symbol.iterator. To use {#each} with an iterable you have to remember to convert it to an Array first, such as {#each [...myIterable] as item}. (This conversion probably does more work too, but I’m more concerned with developer ergonomics than performance or overhead.)

#894 muddies the water because Object properties aren’t iterable. As that issue concludes, the proper way to iterate over properties is by getting an iterable from Object.keys() or the newer Object.entries().

#3225 illustrates the problem of having to sniff out the type of iterable before looping over it. Set is not Array-like by design but is iterable.

Describe the solution you'd like

I’d expect {#each} to work with any iterable. Array would work as it does today because Array instances are iterable. Duck typed Array-like objects would be the outlier. However, converting an Array-like object to an iterable is trivial and could happen under-the-covers.

Describe alternatives you've considered

{#each [...myIterable] as item} works, but it’s an extra conversion to remember. Iterators were designed just for this purpose.

How important is this feature to you?

In the grand scheme of things, not super important, but one of those things you bump up against periodically.

Additional context

Here a reproduction.

@ghost
Copy link

ghost commented Jan 20, 2020

Duplicate of #2968 See the discussion there.

You can achieve the same thing using proxy object (yeah, @pngwn, I know you're not a fan 😛 ).

See my implementation of Python-like range function:
https://svelte.dev/repl/8b0a6bf59a9e4bd5a0e2fce4c7a06e86?version=3.17.1

@jmakeig
Copy link
Author

jmakeig commented Jan 20, 2020

This isn’t about syntax sugar for ranges. This is about supporting the core iteration protocol of JavaScript. Once you do that, ranges and iterating over Object properties would be trivial. There might be benefits to additional syntax sugar, but that’s a separate issue.

@jmakeig
Copy link
Author

jmakeig commented Jan 20, 2020

Things that are iterable out-of-the-box in ES2015:

  • Array
  • Map
  • Set
  • Generator functions (i.e. function*)
  • TypedArray
  • String (regrettably)

…and any custom object that has a Symbol.iterator property. Iterators serve a similar purpose to duck-typing with .length, but are much more flexible and built into the language.

@ghost
Copy link

ghost commented Jan 20, 2020

Yeah, I get it... please read the entire thread of the mentioned issue.
The crux of the issue is, as you say, the assumption of array-based objects.
This doesn't change the fact that it is well-known limitation and hence a duplicate issue.

@jmakeig
Copy link
Author

jmakeig commented Jan 20, 2020

Sure, but your actual iterable proposal is nestled deep within a thread about Python-style ranges.

I don't think people should need to understand the iterator protocol to write a for loop in a svelte template. Iterators are also slow (although they have improved over time and will continue to do so).

I guess I don’t understand the above assertion from @pngwn. Iterators are fundamental to JavaScript. There’s nothing to understand unless you don’t use Arrays. In that case, you’ve already made an explicit choice. Converting to an Array is the thing you have to remember.

@pngwn
Copy link
Member

pngwn commented Jan 20, 2020

Because it was in response to a custom function that iterates. To write such a function you would need to understand the protocol. We weren’t discussing changes to the compiler at that point.

@Conduitry
Copy link
Member

I don't really see how Svelte could write compiled code that would be any more efficient than something that's equivalent to {#each [...iter] as ...} now. I don't think we can reasonably avoid internally creating an array of everything in the iterable. I can see the argument for supporting this if it did something clever, but less so if it's cosmetic thing letting you save five characters.

@jmakeig
Copy link
Author

jmakeig commented Jan 22, 2020

This is about developer ergonomics, not performance. I tried to do an ‘{#each}’ on a ‘Set’ and it didn’t work. I was assuming ‘for…of’ semantics because that’s built into the language.

@Conduitry
Copy link
Member

Allowing the same syntax to be used for both array-like objects and iterables is something that's come up before, and has been rejected before because it adds an extra check to everyone's each blocks, whether they want to use this feature or not.

It does seem helpful and practical though to add a dev mode runtime check to ensure that the object passed to an each is indeed an array-like - with a specific human-readable error message if not. It wouldn't help having to type the extra characters, but it would help with confusing runtime behavior.

@jmakeig
Copy link
Author

jmakeig commented Jan 22, 2020

Array-like, but not iterable is the odd ball here. I don’t think it would be unreasonable for a developer to have to convert those to iterables to be used out-of-the-box with {#each}.

NodeList is a good illustration. Developers used to have to remember when a NodeList behaved like an Array and when it didn’t. For the most common case of iterating over all of the items in a NodeList, you had to remember to convert it to an Array or use your own for loop. Browser vendors have since made NodeList instances iterable to align with the overall direction of the language. I’d expect that’s where they’ll invest their performance tuning as well.

If someone can point me to the relevant section in the compiler, I’d love to take a crack at a PR. Mostly for my own edification; it doesn’t sound like there’s much support here. (That's OK and an expected part of the process.)

Thanks, all.

@mikebeaton
Copy link

mikebeaton commented Feb 13, 2020

I don't really see how Svelte could write compiled code that would be any more efficient than something that's equivalent to {#each [...iter] as ...} now.

@Conduitry, Sorry, I know this is closed, but is that right? Surely:

  if (Array.isArray(things)) {
    for (; i < things.length; i += 1) {
      callback(things[i], i, i);
    }
  } else if (typeof Symbol !== 'undefined' && Symbol.iterator in things) {
    for (const thing of things) {
      callback(thing, i, i++);
    }
  }

taken directly from #894 (comment) is already a lot more efficient for iterables? And only slightly less efficient than currently, for arrays? (I mean surely, really not a significant impact - neither in bundle size nor speed - especially not for the gain of making the API much less surprising, as well as more efficient for what might be a common, growing use-case.)

@absolux
Copy link

absolux commented May 29, 2021

I don't really see how Svelte could write compiled code that would be any more efficient than something that's equivalent to {#each [...iter] as ...} now.

@Conduitry, @mikebeaton, using a for..of loop can handle any kind of supported iterables including arrays.
Array-like objects should implement the Iterable interface or provide Iterator factories.

the compiler now compiles #each block to a simple for loop

for (let i = 0; i < each_value.length; i += 1) {
  each_blocks[i] = create_each_block(get_each_context(ctx, each_value, i));
}

but, it can compile to for..of instead.

let  i = 0;

for (let x of each_value) {
  each_blocks[i] = create_each_block(get_each_context(ctx, x, i++));
}

no check, no conditionals, no worries...

@mikebeaton
Copy link

This sounds right, and is obviously related to this comment #894 (comment) by @Rich-Harris . May be an issue around support in different versions of JS? I know @sw-yx is interested in this too.

@aradalvand
Copy link

aradalvand commented Dec 6, 2021

This should've been implemented honestly, what is wrong with absolux's suggestion? Just compile {#each ...} into for...of. Sounds simple. Are there any other concerns?

@SBHattarj
Copy link

I would also like to see each support iterable. As for large number of elements it is not prefarable to convert the iterable to an array and sometimes it can be problamatic.

@aradalvand
Copy link

@jmakeig Could you please consider reopening this issue?

@jmakeig
Copy link
Author

jmakeig commented Apr 1, 2023

The people have spoken?

@jmakeig jmakeig reopened this Apr 1, 2023
@ngtr6788
Copy link
Contributor

ngtr6788 commented Apr 3, 2023

I noticed another issue which possibly is similar to this one, #7425.

@dummdidumm
Copy link
Member

Yes, thank you - closing as duplicate of #7425

@dummdidumm dummdidumm closed this as not planned Won't fix, can't repro, duplicate, stale Apr 3, 2023
@jmakeig
Copy link
Author

jmakeig commented May 17, 2023

Yes, thank you - closing as duplicate of #7425

Or vice versa. Sigh.

@aradalvand
Copy link

aradalvand commented May 17, 2023

@dummdidumm This issue is 2 years older and has more upvotes than #7425.
Shouldn't #7245 have been closed in favor of this one, rather than the other way around?

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

No branches or pull requests

9 participants