Add support for spreads in literal

[donny-dont]

Adds a trailing parameter which allows the addition of a spread expression. The Expression isn't wrapped in a spread automatically because there are two types of spreads.

[donny-dont]

Opened this early to get feedback on it. Will do the other literals if everything looks good.

[natebosch]

Hmm, I'm not sure if we've ever had a discussion about how much of the Dart syntax to try to support in this package.

My personal opinion is that it's better to not try to support all the latest syntax within methods, but to focus on the syntax which impact the API surface area.

Do we think it's a significant drawback to require cascading a addAll call instead of allowing a spread?

[srawlins]

Hmm, I'm not sure if we've ever had a discussion about how much of the Dart syntax to try to support in this package.

Agreed. For example, we could just support the basic building blocks that more advanced syntax gets desugared into.

That being said, there might be performance implications to using spread? Unless it just gets desugared into addAll, haha.

Also you can spread const lists, but cannot addAll into one. E.g. const x = [1, 2, 3]; const y = [...x, 4, 5, 6];.

[donny-dont]

There is one potential concern with this approach, which is that you can't produce something where the spread operator is earlier, which technically does allow you to override keys. (granted, this is kinda horrible to do, but theoretically could have its uses)

I can't think of a very clean way to do this with any kind of sentinel value and I kinda feel if you wanted to do that you're on your own.

Also you can spread const lists, but cannot addAll into one. E.g. const x = [1, 2, 3]; const y = [...x, 4, 5, 6];.

Right that is one of the reasons for wanting to do the spread here.

[jakemac53]

For code generation I do think its also probably fairly common to want to generate const map literals, and being able to spread other ones into there seems reasonable too?

[natebosch]

Yes, the const use case is compelling.

I think we should explore an API which allows any element within a literal to be a spread. If it leads to it being possible to do nonsense like writing a spread as a statement or expression outside a literal - that's fine.

An API which allows a single spread element as the trailing element of a collection feels pretty limiting. I think allowing both a leading and trailing argument would be general enough to let someone build up to the collection they are after with spreads, but it would be complex.

[kevmoo]

Worth a changelog entry?

[donny-dont]

Yes, the const use case is compelling.

I think we should explore an API which allows any element within a literal to be a spread. If it leads to it being possible to do nonsense like writing a spread as a statement or expression outside a literal - that's fine.

An API which allows a single spread element as the trailing element of a collection feels pretty limiting. I think allowing both a leading and trailing argument would be general enough to let someone build up to the collection they are after with spreads, but it would be complex.

Something like 👇 where mapSpread just is a BS sentinel value that we then look for?

literalMap(<Object?, Object?>{
  mapSpread(): refer(m1)..spread,
  literalString('foo'): literalNum(1),
  mapSpread(): refer(m2).spread,
  literalString('bar'): literalNum(2),
  mapSpread(): refer(m3).nullSafeSpread,
});

Just spit balling here since I feel like you could do the spread with the other literals already. I'm also assuming the Map is iterated in insertion order.

Worth a changelog entry?

I had assumed it would I just wanted feedback prior to making any.

[donny-dont]

Ok @natebosch @jakemac53 @srawlins is this the API you're looking for?

     literalMap({
        literalSpread(): refer('one'),
        2: refer('two'),
        literalNullSafeSpread(): refer('three'),
        refer('Map').newInstance([]): null,
      })

[natebosch]

is this the API you're looking for?

I think that could work.

We could also consider something like

literalMapEntries([
  mapSpread(refer('one'),
  mapEntry(2, refer('two')),
  nullSafeMapSpread(refer('three')),
]);

Looking forward to seeing thoughts from other folks.

[donny-dont]

Any thoughts @srawlins @jakemac53 ?

[jakemac53]

I think I prefer the literalMap approach with placeholder keys for spreads

[donny-dont]

@natebosch @srawlins anything? Got some time to work on any changes if I can get a final direction.

[natebosch]

Let's go with the literalSpread(): refer('one') syntax as you first suggested.

[donny-dont]

@natebosch would we be doing a 4.5.0 then? Just have the CHANGELOG entry to add.

[natebosch]

@natebosch would we be doing a 4.5.0 then? Just have the CHANGELOG entry to add.

Yes, a changelog and bump to 4.5.0 should be all that's needed.

[donny-dont]

@natebosch changelog added so I think this is ready to go. Thanks everyone!