Skip to content

Conversation

@darvld
Copy link
Member

@darvld darvld commented Apr 12, 2025

Ready for review Powered by Pull Request Badge

Summary

Introduces a pure-Kotlin implementation of the Web Streams standard, meant to replace the polyfills currently in place.

The implementation is functional but far from optimal, as it was made to be as close as possible to the specification, which resulted in JavaScript-style code that does not fully utilize the capabilities of Kotlin. In the future, a more optimal and streamlined (pun intended) implementation will be added, likely using coroutine primitives such as channels to avoid the current boilerplate.

  • Interface definitions for the streams API
  • Deduplicate stream API interfaces
  • Readable streams
    • Default controller/reader
    • BYOB controller/reader
    • Guest ReadableStreamSource wrapper
  • Writable streams
    • Default controller/writer
    • Guest WritableStreamSink wrapper
  • Transform streams
    • Default controller
    • Guest TransformStreamTransformer wrapper
  • Basic test suite
  • Disable streams polyfill

Note

A companion PR at runtime/382 removes the polyfill from the runtime image.

@darvld darvld self-assigned this Apr 12, 2025
@darvld darvld added the 🚧 WIP Works-in-progress. Blocks merge label Apr 12, 2025
@sgammon sgammon changed the title feat: native streams [wip] feat: native streams Apr 12, 2025
Copy link
Member

@sgammon sgammon left a comment

Choose a reason for hiding this comment

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

docs and approach look solid

@sgammon
Copy link
Member

sgammon commented Apr 12, 2025

CI failures

/home/runner/work/elide/elide/packages/graalvm/src/main/kotlin/elide/runtime/gvm/internals/intrinsics/js/webstreams/ReadableDefaultStream.kt:104:16: The caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled. [TooGenericExceptionCaught]

Feel free to run :packages:graalvm:detektBaseline

Execution failed for task ':packages:graalvm:spotlessKotlinCheck'.

Format with :packages:graalvm:spotlessApply, modify the CI job to drop this until it's non-draft. Honestly I might just remove spotless anyway.

@sgammon sgammon linked an issue Apr 12, 2025 that may be closed by this pull request
@darvld darvld force-pushed the feat/native-streams branch from 1d36241 to 9d8869e Compare April 19, 2025 01:47
@darvld darvld force-pushed the feat/native-streams branch 3 times, most recently from 727b582 to b3e8ed0 Compare May 13, 2025 01:26
@darvld darvld force-pushed the feat/native-streams branch from b3e8ed0 to d89036e Compare May 24, 2025 02:34
@codecov
Copy link

codecov bot commented May 24, 2025

Codecov Report

Attention: Patch coverage is 37.01380% with 1004 lines in your changes missing coverage. Please review.

Project coverage is 42.82%. Comparing base (a30547a) to head (d41a8ca).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...als/intrinsics/js/webstreams/ReadableByteStream.kt 28.80% 282 Missing and 27 partials ⚠️
.../intrinsics/js/webstreams/WritableDefaultStream.kt 30.57% 140 Missing and 28 partials ⚠️
.../intrinsics/js/webstreams/ReadableDefaultStream.kt 46.62% 60 Missing and 19 partials ⚠️
...intrinsics/js/webstreams/TransformDefaultStream.kt 31.68% 59 Missing and 10 partials ⚠️
...m/internals/intrinsics/js/webstreams/StreamPipe.kt 27.38% 53 Missing and 8 partials ⚠️
...intrinsics/js/webstreams/ReadableStreamWrappers.kt 0.00% 58 Missing ⚠️
...als/intrinsics/js/webstreams/ReadableStreamBase.kt 50.60% 28 Missing and 13 partials ⚠️
...de/runtime/intrinsics/js/stream/QueuingStrategy.kt 17.64% 28 Missing ⚠️
.../js/webstreams/WritableStreamDefaultWriterToken.kt 53.44% 22 Missing and 5 partials ⚠️
...in/kotlin/elide/runtime/intrinsics/js/JsPromise.kt 58.00% 16 Missing and 5 partials ⚠️
... and 21 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1365      +/-   ##
==========================================
- Coverage   43.13%   42.82%   -0.32%     
==========================================
  Files         647      676      +29     
  Lines       29157    30631    +1474     
  Branches     3928     4258     +330     
==========================================
+ Hits        12577    13117     +540     
- Misses      15181    15964     +783     
- Partials     1399     1550     +151     
Flag Coverage Δ
jvm 42.82% <37.01%> (-0.32%) ⬇️
lib 42.82% <37.01%> (-0.32%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ntrinsics/js/webstreams/ReadableStreamIntrinsic.kt 100.00% <100.00%> (+75.00%) ⬆️
...ics/js/webstreams/ReadableStreamReaderTokenBase.kt 100.00% <100.00%> (ø)
...trinsics/js/webstreams/TransformStreamIntrinsic.kt 100.00% <100.00%> (ø)
...ntrinsics/js/webstreams/WritableStreamIntrinsic.kt 100.00% <100.00%> (ø)
...time/gvm/intrinsics/BuildTimeIntrinsicsResolver.kt 100.00% <100.00%> (ø)
...ain/kotlin/elide/runtime/node/fs/NodeFilesystem.kt 68.07% <100.00%> (ø)
...in/elide/runtime/plugins/AbstractLanguagePlugin.kt 37.50% <0.00%> (ø)
...nsics/js/stream/WritableStreamDefaultController.kt 0.00% <0.00%> (ø)
...tlin/elide/runtime/intrinsics/ai/ElideLLMModule.kt 27.38% <0.00%> (ø)
...e/intrinsics/js/stream/ReadableStreamBYOBReader.kt 50.00% <50.00%> (ø)
... and 27 more

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a30547a...d41a8ca. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@darvld darvld removed the 🚧 WIP Works-in-progress. Blocks merge label May 27, 2025
@darvld darvld marked this pull request as ready for review May 27, 2025 19:09
@darvld darvld force-pushed the feat/native-streams branch from d89036e to eb11c36 Compare May 27, 2025 19:10
@darvld darvld changed the title [wip] feat: native streams feat: native streams May 27, 2025
@sgammon sgammon added feature Large PRs or issues with full-blown features lang:javascript Issues relating to JavaScript labels May 27, 2025
@sgammon sgammon added this to Elide May 27, 2025
@sgammon sgammon added this to the Release R16: Beta milestone May 27, 2025
@sgammon sgammon moved this to Done in Elide May 27, 2025
@darvld darvld force-pushed the feat/native-streams branch 3 times, most recently from d2a0195 to 29b9e5d Compare May 28, 2025 16:41
@darvld darvld force-pushed the feat/native-streams branch from 29b9e5d to d41a8ca Compare May 28, 2025 16:47
@sgammon sgammon merged commit 01cbf9c into main May 28, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Large PRs or issues with full-blown features lang:javascript Issues relating to JavaScript

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Native web steams

3 participants