`BinarySource` supporting `ReadableStream` #3411

dillonstreator · 2023-11-28T16:04:43Z

BinarySource supporting ReadableStream to better enable streaming

vercel · 2023-11-28T16:04:49Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
medplum-www	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 29, 2023 0:20am

2 Ignored Deployments

Name	Status	Preview	Comments	Updated (UTC)
medplum-app	⬜️ Ignored (Inspect)	Visit Preview		Nov 29, 2023 0:20am
medplum-storybook	⬜️ Ignored (Inspect)	Visit Preview		Nov 29, 2023 0:20am

vercel · 2023-11-28T16:04:51Z

@dillonstreator is attempting to deploy a commit to the Medplum Team on Vercel.

A member of the Team first needs to authorize it.

sweep-ai · 2023-11-28T16:05:48Z

Apply Sweep Rules to your PR?

Apply: All new business logic should have corresponding unit tests.
Apply: Refactor large functions to be more modular.
Apply: Add docstrings to all functions and file headers.

packages/core/src/client.test.ts

ThatOneBro

Overall I think this will be a great change. Just one comment about using ReadableStream instead of Readable so that this will work in the browser as well 👍

packages/core/src/client.test.ts

packages/core/src/client.ts

…environment

packages/core/test.setup.cjs

packages/core/src/client.ts

ThatOneBro · 2023-11-28T22:44:44Z

Overall this looks really good! I'm excited to get this in

packages/core/package.json

Co-authored-by: Derrick Farris <[email protected]>

packages/core/src/client.test.ts

ThatOneBro · 2023-11-28T23:36:01Z

Thanks @dillonstreator! Going to merge once all the tests pass 👍 great addition

dillonstreator · 2023-11-29T16:05:44Z

@ThatOneBro This PR should be reverted.
I just did some local testing with this and it is not working as intended. xhr.send() can't be repeatedly called with chunks of the payload.
I'm surprised to find very little online about streaming uploads with XMLHttpRequest. Understandably, most efforts appear to be placed into the fetch standard. As I understand it, we couldn't easily support progress tracking with fetch which is why we fallback to XMLHttpRequest.

Additionally, we can't really stream & report progress (relative to the total payload size) unless we have some mechanism of knowing the size of the stream beforehand. With that, if someone provides a ReadableStream as data and an onProgress, should onProgress just no-op and the client use fetch?

I saw this doc comment that @codyebberson & @ThatOneBro wrote about a longer term approach to handling fetching

medplum/packages/docs/docs/contributing/upgrading-dependencies.md

Lines 61 to 70 in c80d69d

 ### node-fetch 

 At the time of this writing, we use node-fetch version 2.6.7. The developers of node-fetch have admirably taken the position that ESM-only libs are the future. The version 3 series is ESM-only. Unfortunately, our current server configuration is not yet compatible with ESM-only modules (despite many attempts). 

 Therefore, for now, we keep node-fetch pinned at the latest version in the version 2 series, which continues to receive security fixes. 

 In the future, there are 2 possibilities: 

 - As the ESM ecosystem matures, we eventually support an ESM-only dependency. 

 - For completely unrelated reasons (i.e., upload progress monitoring), we may move to [Axios](https://www.npmjs.com/package/axios), which would eliminate this exception entirely.

Maybe a good time to continue the discussion of moving to Axios?

ThatOneBro · 2023-11-29T17:55:12Z

The node-fetch dependency is something we should definitely look at again. As of Node 18, global fetch is now native to Node and so this dependency might be unnecessary since we recently set the minimum Node version to 18.

As for streaming, yeah you're right. I totally forgot that XHR has never supported file streaming. We could do multipart uploads (using Content-Range) but that wouldn't be quite the same as file streaming and would probably not have the desired performance.

In terms of Axios, I'm skeptical it will improve things much on the browser-side of things in regards to streaming. It looks like there are two backends for Axios, namely node, which uses node:http which does support streaming if I remember correctly, but I don't think it does it the same way that fetch does (uses HTTP/1.1 Chunked Transfer Encoding maybe?), and the second is xhr. See: https://github.com/axios/axios/tree/v1.x/lib/adapters

So in the browser it is again probably not possible to do the file streaming while using axios. I think we will have to find a solution with fetch that still has upload progress (surely that exists by now... right?).

I did find this TransformStream solution, which may work, but I haven't tested it extensively. The example seems promising though:
https://stackoverflow.com/a/69400632
There is mention that TransformStream is not supported in browsers other than Chrome yet, but seems support has improved:
https://developer.mozilla.org/en-US/docs/Web/API/TransformStream

I'll probably play around with it later today.

In the meantime, yes we should revert this commit I guess and come back when we have a better solution. If you want to submit a PR reverting it I'll get that in. 👍

Thank you for your diligence in following up on this

This reverts commit 7c80d0f.

dillonstreator · 2023-11-29T18:08:35Z

Opened the revert #3421

Ah didn't know about the minimum Node version of 18 so ya agreed node-fetch is likely irrelevant.

Saw the same SO post and agree it seems quite promising.

ThatOneBro · 2023-11-29T18:37:05Z

Yeah for sure! If you want to test it out, feel free. I think I'd also like to know @codyebberson's take on this too

This reverts commit 7c80d0f.

Fixes #3391 - setup git in preapare-release-yml (#3448) Create example bots that use a shared code library (#3441) Fixes #2789 - support 'delete' subscriptions with bots (#3426) (#3445) Fixes #3391 - github action for prepare-release.sh (#3443) Revert "Fixes #2789 - support 'delete' subscriptions with bots (#3426)" (#3444) Update AWS instructions (#3437) Bump @adobe/css-tools from 4.3.1 to 4.3.2 (#3440) Fix ValidBot warnings (#3438) Fixes #3404 - add Referrer-Policy and Permissions-Policy headers (#3436) Fixes #2789 - support 'delete' subscriptions with bots (#3426) Agent source cleanup (#3431) Stop reviving removed meta properties (#3432) Create docs for building questionnaires (#3393) feat(@medplum/core) Add `AbortSignal` support to client's `createBinary` and `createAttachment` methods (#3420) Fixes #3401 - Handle no AWS credentials in AWS init tool (#3418) Revert "`BinarySource` supporting `ReadableStream` (#3411)" (#3421) Add bulleted list of SuperAdmin priveleges (#3408) Add a note that config must be updated locally when making changes (#3410) Improve linking for using binary data (#3406) Fixes #3413 - set meta.author when updating access policy (#3419) `BinarySource` supporting `ReadableStream` (#3411) Handle non-string code in formatCoding (#3415) Fixes #3414 - use ESM in example apps (#3416) Fixes #3389 - add vercel.json config to examples (#3417) feat(fhircast): add `heartbeat` event (#3350) Add note about graphql respecting access policies (#3403) Fixes #3395 - Build scripts handle no git repo (#3400) Dependency upgrades (#3397) Add callout for batch vs transaction bundles (#3399) Fixes #3395 - Build scripts handle no git repo (#3396) Improve examples in the include search docs (#3359) QuestionnaireBuilder Autosave (#3253) chore(package.json): add `private: true` to root (#3390) Docs: FHIR Profile Creation & Search (#3357)

dillonstreator requested a review from a team as a code owner November 28, 2023 16:04

vercel bot deployed to Preview – medplum-www November 28, 2023 16:11 View deployment

vercel bot deployed to Preview – medplum-www November 28, 2023 17:06 View deployment

dillonstreator commented Nov 28, 2023

View reviewed changes

packages/core/src/client.test.ts Outdated Show resolved Hide resolved

ThatOneBro requested changes Nov 28, 2023

View reviewed changes

packages/core/src/client.test.ts Outdated Show resolved Hide resolved

packages/core/src/client.ts Outdated Show resolved Hide resolved

dillonstreator force-pushed the binary-source-readable-stream branch from 2e8ca54 to 955f29d Compare November 28, 2023 22:22

vercel bot deployed to Preview – medplum-www November 28, 2023 22:32 View deployment

dillonstreator force-pushed the binary-source-readable-stream branch from 955f29d to a9df4b3 Compare November 28, 2023 22:35

dillonstreator added 3 commits November 28, 2023 16:36

BinarySource supporting Readable

c97e92e

prefix node imports with node: and move to top

47d4540

use ReadableStream and WritableStream to support both node & browser …

b27e68c

…environment

dillonstreator force-pushed the binary-source-readable-stream branch from a9df4b3 to b27e68c Compare November 28, 2023 22:39

dillonstreator requested a review from ThatOneBro November 28, 2023 22:40

dillonstreator changed the title ~~BinarySource supporting Readable~~ BinarySource supporting ReadableStream Nov 28, 2023

ThatOneBro requested changes Nov 28, 2023

View reviewed changes

packages/core/test.setup.cjs Show resolved Hide resolved

packages/core/src/client.ts Outdated Show resolved Hide resolved

ThatOneBro requested changes Nov 28, 2023

View reviewed changes

packages/core/package.json Outdated Show resolved Hide resolved

vercel bot deployed to Preview – medplum-www November 28, 2023 22:49 View deployment

dillonstreator and others added 2 commits November 28, 2023 16:49

Update packages/core/package.json

1489e6b

Co-authored-by: Derrick Farris <[email protected]>

remove no-void ignore

7284420

ThatOneBro reviewed Nov 28, 2023

View reviewed changes

packages/core/src/client.test.ts Show resolved Hide resolved

keep test for progress w/ string to ensure no regression

dd7c95f

dillonstreator requested a review from ThatOneBro November 28, 2023 23:00

update package-lock.json

990e3cf

ThatOneBro approved these changes Nov 28, 2023

View reviewed changes

ThatOneBro assigned dillonstreator Nov 28, 2023

ThatOneBro added this to the November 30, 2023 milestone Nov 28, 2023

ThatOneBro enabled auto-merge November 28, 2023 23:45

ThatOneBro added this pull request to the merge queue Nov 29, 2023

Merged via the queue into medplum:main with commit 7c80d0f Nov 29, 2023
13 checks passed

dillonstreator added a commit to dillonstreator/medplum that referenced this pull request Nov 29, 2023

Revert "BinarySource supporting ReadableStream (medplum#3411)"

1e0c11e

This reverts commit 7c80d0f.

dillonstreator mentioned this pull request Nov 29, 2023

Revert "BinarySource supporting ReadableStream (#3411)" #3421

Merged

github-merge-queue bot pushed a commit that referenced this pull request Nov 29, 2023

Revert "BinarySource supporting ReadableStream (#3411)" (#3421)

d670fe3

This reverts commit 7c80d0f.

reshmakh added the fhir-datastore Related to the FHIR datastore, includes API and FHIR operations label Dec 1, 2023

medplumbot mentioned this pull request Dec 2, 2023

Release Version 2.1.21 #3450

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`BinarySource` supporting `ReadableStream` #3411

`BinarySource` supporting `ReadableStream` #3411

dillonstreator commented Nov 28, 2023 •

edited

Loading

vercel bot commented Nov 28, 2023 •

edited

Loading

vercel bot commented Nov 28, 2023

sweep-ai bot commented Nov 28, 2023

ThatOneBro left a comment

ThatOneBro commented Nov 28, 2023

ThatOneBro commented Nov 28, 2023

dillonstreator commented Nov 29, 2023 •

edited

Loading

ThatOneBro commented Nov 29, 2023

dillonstreator commented Nov 29, 2023

ThatOneBro commented Nov 29, 2023

BinarySource supporting ReadableStream #3411

BinarySource supporting ReadableStream #3411

Conversation

dillonstreator commented Nov 28, 2023 • edited Loading

vercel bot commented Nov 28, 2023 • edited Loading

vercel bot commented Nov 28, 2023

sweep-ai bot commented Nov 28, 2023

Apply Sweep Rules to your PR?

ThatOneBro left a comment

Choose a reason for hiding this comment

ThatOneBro commented Nov 28, 2023

ThatOneBro commented Nov 28, 2023

dillonstreator commented Nov 29, 2023 • edited Loading

ThatOneBro commented Nov 29, 2023

dillonstreator commented Nov 29, 2023

ThatOneBro commented Nov 29, 2023

`BinarySource` supporting `ReadableStream` #3411

`BinarySource` supporting `ReadableStream` #3411

dillonstreator commented Nov 28, 2023 •

edited

Loading

vercel bot commented Nov 28, 2023 •

edited

Loading

dillonstreator commented Nov 29, 2023 •

edited

Loading