Skip to content

Conversation

@He-Pin
Copy link
Member

@He-Pin He-Pin commented Nov 4, 2025

Motivation:
refs: #2416

It's named just in Flux and ambArray in Flowable.

@mdedetrich want the Scala method to be Source.apply, and then we can call Source(1,2,3,4), wdyt @pjfanning .

if we do that, then Source(Array(1,2,3) will emit 1,2,3, and Source(Array(1,2,3), Array(1,2,3)will emitArray(1,2,3), Array(1,2,3)`

@He-Pin He-Pin added this to the 2.0.0-M2 milestone Nov 4, 2025
@He-Pin He-Pin added the t:stream Pekko Streams label Nov 4, 2025
@mdedetrich
Copy link
Contributor

mdedetrich commented Nov 4, 2025

Just to add onto what I said, I was apprehensive adding the Source.elements version because Source(List(1,2,3)) is shorter than Source.elements(1,2,3) at which point Source.elements is kind of pointless. So to me it only makes sense to add varargs as an overloaded apply method.

@raboof Would be good to get your opinion on this as well.

@pjfanning
Copy link
Member

Isn't this the same as #2416?

@mdedetrich
Copy link
Contributor

mdedetrich commented Nov 5, 2025

Isn't this the same as #2416?

Yes it is but for some context, when I reviewed this feature I questioned the premise of Source.elements given that Source(List(1,2,3)) is even shorter than Source.elements(1,2,3) and hence I suggested to use Source.apply as an overloaded varargs which would allow you to do Source(1,2,3).

However as @He-Pin pointed out, this does create some discrepencies on how sources are created

@pjfanning
Copy link
Member

I'm fairly neutral on this. If the Source.apply can't be implemented to do what we want then Source.elements is fine with me.

@mdedetrich
Copy link
Contributor

mdedetrich commented Nov 5, 2025

I'm fairly neutral on this. If the Source.apply can't be implemented to do what we want then Source.elements is fine with me.

To be clear, Source.apply with varargs works fine for the intended purpose i.e.

/**
 * Create a `Source` from the given elements.
 *
 * @since 1.3.0
 */
@varargs
@SafeVarargs
@SuppressWarnings(Array("varargs"))
def apply[T](elements: T*): javadsl.Source[T, NotUsed] = {
  if (elements.isEmpty) {
    empty()
  } else if (elements.length == 1) {
    single(elements.head)
  } else {
    new Source(scaladsl.Source(elements))
  }
}

works totally fine, allowing you to do Source(1,2,3). The issue as @He-Pin pointed out is when you do weird things like Source(Array(1, 2, 3), Array(4,5,6)). In this case you would get 2 elements in the stream with both being Array's which can be seen as weird behavior.

In my personal view I don't see this as an issue considering that Source's sending raw Array's as elements is almost never what people want however it is still an inconsistency.

On a broader point, for this very reason is why a lot of Scala libraries have avoided using varargs in constructors entirely. The language is succint/expressive enough that Scala users are fine with Source(List(1,2,3))

@mdedetrich
Copy link
Contributor

mdedetrich commented Nov 5, 2025

Actually @He-Pin , I think you might be able to solve this issue by adding def apply[T](elements: Array[T]*): javadsl.Source[T, NotUsed] and def apply[T](elements: immutable.Iterable[T]*): javadsl.Source[T, NotUsed] which would then mean that Source(Array(1, 2, 3), Array(4,5,6)) will translate to a stream that emits 1,2,3,4,5,6 as distinct individual elements. The problem here would be in the very rare exception that someone wants to send raw Array's in a stream they are effectively unable to or they need to use Source.single with some extra operators.

@He-Pin
Copy link
Member Author

He-Pin commented Nov 5, 2025

I would like to make this just a javadsl if you think Source. elements is too long for Scala to avoid the ambiguity

@mdedetrich
Copy link
Contributor

mdedetrich commented Nov 5, 2025

I would like to make this just a javadsl if you think Source. elements is too long for Scala to avoid the ambiguity

For javadsl Source.elements is unambigiously the right answer. I will have a look at other Scala libraries to see what they do and report back

@He-Pin
Copy link
Member Author

He-Pin commented Nov 5, 2025

There is a Source.from(Arrays.asList(...)) in java too.

@pjfanning
Copy link
Member

I don't think a source created with 2 arrays should be automatically flattened but if other source functions automatically flatten inputs then maybe this should too. I would still favour not automatically flattening unless this were a few examples of it already in Pekko.

@He-Pin
Copy link
Member Author

He-Pin commented Nov 5, 2025

Yes, the problem of make elements an apply method in pekko is, the elements(Array(1,2,3)) will not be Source(Array(1,2,3))

@He-Pin
Copy link
Member Author

He-Pin commented Nov 7, 2025

@mdedetrich @pjfanning Should I just remove the scaladsl's elements? and just keep the java one?

@mdedetrich
Copy link
Contributor

Woops forgot about this, will post a question in Scala contributors

@mdedetrich
Copy link
Contributor

Created a thread here https://contributors.scala-lang.org/t/idiomatic-approach-to-apply-with-varargs/7285, lets see what Scala community says

@He-Pin
Copy link
Member Author

He-Pin commented Nov 7, 2025

@mdedetrich @pjfanning Should we rename apply(array:Array[T]) to fromArray and rename Source.elements to Source.apply?

@He-Pin He-Pin modified the milestones: 2.0.0-M2, 2.0.0-M1 Nov 7, 2025
@He-Pin He-Pin changed the title feat: Add Source#elements feat: Add Source#items Nov 8, 2025
@He-Pin
Copy link
Member Author

He-Pin commented Nov 8, 2025

@mdedetrich @pjfanning It's renmaed to items as suggested

@mdedetrich
Copy link
Contributor

@mdedetrich @pjfanning It's renmaed to items as suggested

Added one comment

@He-Pin
Copy link
Member Author

He-Pin commented Nov 10, 2025

@mdedetrich Would you like to take a look at this?

Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@He-Pin He-Pin merged commit f0db8f0 into main Nov 10, 2025
9 checks passed
@He-Pin He-Pin deleted the elements branch November 10, 2025 13:47
He-Pin added a commit that referenced this pull request Nov 10, 2025
(cherry picked from commit f0db8f0)
He-Pin added a commit that referenced this pull request Nov 10, 2025
(cherry picked from commit f0db8f0)
He-Pin added a commit that referenced this pull request Nov 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t:stream Pekko Streams

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants