Readable stream analysis refactor #3231

scorbajio · 2024-01-12T03:27:56Z

This PR looks into options for the issues discussed in #3218.

codecov · 2024-01-12T03:31:08Z

Codecov Report

Attention: 32 lines in your changes are missing coverage. Please review.

Comparison is base (51fc6da) 87.93% compared to head (a6a4984) 87.94%.

Additional details and impacted files

Flag	Coverage Δ
block	`87.52% <ø> (ø)`
blockchain	`91.60% <ø> (ø)`
client	`84.66% <ø> (ø)`
common	`98.25% <ø> (ø)`
devp2p	`82.15% <ø> (ø)`
ethash	`∅ <ø> (∅)`
evm	`76.96% <ø> (ø)`
genesis	`99.98% <ø> (ø)`
rlp	`∅ <ø> (∅)`
statemanager	`86.53% <18.18%> (+0.24%)`	⬆️
trie	`89.63% <92.18%> (-0.13%)`	⬇️
tx	`95.73% <ø> (ø)`
util	`89.13% <ø> (ø)`
vm	`80.83% <ø> (ø)`
wallet	`91.00% <ø> (ø)`

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

holgerd77

Awesome, thanks for exploring so quickly, this generally looks great and will be a substantial improvement on the library. :-) (my new MacOS update killed the emojy usage, lol, at least in Firfox).

Two very general things to discuss around backwards compatibility and Node.js specific imports, more for the whole team (at least the second one).

holgerd77 · 2024-01-15T09:42:19Z

packages/statemanager/src/stateManager.ts

-      const stream = trie.createReadStream()
+    for await (const chunk of stream) {
+      storage[bytesToHex(chunk.key)] = bytesToHex(chunk.value)
+    }


I haven't looked too deeply, a general thing though:

We need to keep the API intact (aka: backwards compatible), so from createReadStream(), so the return type is therefore not allowed to change.

If I interpret the code above correctly (showing a usage example of createReadStream() it does here though (it does, right? so the new stream API is realized with async calls, the old one with events, if I see correctly?).

Just to clarify: I also like the new API a lot better.

We nevertheless need to find a way to remain compatibilty, otherwise things will break for users using the method in the old way.

Two eventual options:

Can we use the web stream API in the way the old one worked (just asking for completeness, I would otherwise assume: likely not)?

If not, we likely need to bite the sour apple and keep the old method "as is", keep the dependency for now, add a new method with an eventually temporary name (createWebReadStream(), createAsyncReadStream(),?) and then we add this whole thing to Breaking/Deprecation Tracking Issue #3216 to remember to fully remove the dependency along the next breaking release round and eventually rename the new method name to the old one.

If we go 2. (guess that's likely?) please also add a @deprecated note to the code docs of the old method.

Yes, it seems like the web API differs from readable-stream in that it uses async calls instead of events. I just took a deeper look and can confirm that there isn't an option to use events with the new web API, so I added the @deprecated note and made both options available to eventually update it in the future with a breaking release.

holgerd77 · 2024-01-15T09:47:17Z

packages/trie/src/trie.ts

@@ -43,6 +43,8 @@ import type {
 import type { OnFound } from './util/asyncWalk.js'
 import type { BatchDBOp, DB, PutBatch } from '@ethereumjs/util'
 import type { Debugger } from 'debug'
+// eslint-disable-next-line implicit-dependencies/no-implicit
+import type { ReadableStream } from 'node:stream/web'


Maybe also for others to chime in (@jochem-brouwer @acolytec3, everyone): will this type import affect browser and or other runtime support (bun, deno) in any way?

We generally have the policy to not add Node.js specific package imports. Does this also go for types? I am a bit unsure here. For web it shouldn't make a difference since web is using JavaScript (?). For bun I tested a trie package test run with bunx vitest run test. This does work, not sure if vitest "shields" something here.

I am totally undecided. Would it eventually make sense to spare out this import and rather go for any? Or are we on the safe side here?

I am no browser expert here but I would assume that bun / deno do not have access to this node:stream/web package, however your bunx vitest run test indeed seems to pass. However I would assume that this would fail on browser builds, which does not seem to be the case (and the CI browser tests also pass). I have tried bunx vitest run --config=./vitest.config.browser.ts --browser.name=chrome --browser.headless, this fails all tests:

Error: Module "events" has been externalized for browser compatibility. Cannot access "events.EventEmitter" in client code. See https://vitejs.dev/guide/troubleshooting.html#module-externalized-for-browser-compatibility for more details.

However this does not seem to be related to ReadableStream 🤔

The point here is: since this is a web API, this import should just not be needed in the browser, and the code work nevertheless. I’m am just not sure how these cases are handled in practice. It would (eventually?) e.g. make sense if browsers just „overlook“ (aka: ignore) such an import.

In this case it is even a type import which should go away for JavaScript anyhow (re-read my message above). So generally I would assume we are on the safe side here.

@scorbajio maybe do add a comment above this import shortly explaining why this can go as an exception and stating that we normally do not do Node.js imports in our libs (not that this serves other devs as a reference to conclude: Node.js allowed). A date for the comment also can’t hurt to get some context.

(and the alternative is still that we go for „any“ here, might also be a justified choice in this case)

I added a note detailing the choice to deviate from our import policy and kept the typing.

Thanks, great comment! 🙏

Also have added to the deprecation issue #3216 as well

Hello @scorbajio @holgerd77 , we current have an issue building for the browser using webpack:

ERROR in node:stream/web Module build failed: UnhandledSchemeError: Reading from "node:stream/web" is not handled by plugins (Unhandled scheme). Webpack supports "data:" and "file:" URIs by default. You may need an additional plugin to handle "node:" URIs.

could this be related to this change and if yes what would you recommend to make it build for the browser?
Thanks very much!

@yann300 ah, we were arguing if this might bite us or not 😬 😛 (as seen above), seems we now have the confirmation.

@scorbajio can you prepare a short PR where we replace this with any and remove the import, as discussed above? That would be nice!

@yann300 Guess we'll make some bugfix releases relatively soon, maybe give us a couple more days, there might be 2-3 other things we would want to include as well.

I've raised #3280 to address this.

@yann300 Would you be able to provide the steps to reproduce this webpack build error so we can explore the problem and potential fixes?

…umjs/ethereumjs-monorepo into readable-stream-analysis-refactor

This reverts commit baf67f4.

This reverts commit 02b8845.

holgerd77

Nice, LGTM!

yann300 · 2024-02-13T07:58:27Z

packages/trie/src/util/readStream.ts

@@ -1,3 +1,5 @@
+// eslint-disable-next-line implicit-dependencies/no-implicit
+import { ReadableStream } from 'node:stream/web'


fyi @scorbajio @holgerd77 the import is also present here.

scorbajio added PR state: WIP dependencies Pull requests that update a dependency file package: mpt labels Jan 12, 2024

Experiment with importing webstreams

a7b917d

scorbajio force-pushed the readable-stream-analysis-refactor branch from f0250c1 to a7b917d Compare January 12, 2024 03:28

scorbajio added 6 commits January 11, 2024 20:41

Suppress implicit-import eslint error

e0ac781

Replace readable-stream with web stream api

8e14f6e

Remove unnecessary null check

db0d02b

Update package-lock and dependencies

02b8845

Fix test

5be8db1

Merge branch 'master' into readable-stream-analysis-refactor

330a3d7

scorbajio added PR state: needs review and removed PR state: WIP labels Jan 12, 2024

holgerd77 reviewed Jan 15, 2024

View reviewed changes

scorbajio added 10 commits January 16, 2024 16:47

Make nonbreaking

68d6aeb

Merge branch 'readable-stream-analysis-refactor' of github.com:ethere…

121fd04

…umjs/ethereumjs-monorepo into readable-stream-analysis-refactor

Merge branch 'master' into readable-stream-analysis-refactor

964d9af

Use async stream in statemanager

962c602

Merge branch 'readable-stream-analysis-refactor' of github.com:ethere…

7b1e5da

…umjs/ethereumjs-monorepo into readable-stream-analysis-refactor

Update package-lock file and dependencies

baf67f4

Include comment to detail deviation from import policy

9911d92

Revert "Update package-lock file and dependencies"

85e7af8

This reverts commit baf67f4.

Revert "Update package-lock and dependencies"

868c3bb

This reverts commit 02b8845.

Update package-lock

a6a4984

holgerd77 mentioned this pull request Jan 17, 2024

Breaking/Deprecation Tracking Issue #3216

Open

21 tasks

holgerd77 approved these changes Jan 17, 2024

View reviewed changes

holgerd77 merged commit a8f326a into master Jan 17, 2024
45 of 46 checks passed

holgerd77 deleted the readable-stream-analysis-refactor branch January 17, 2024 12:22

scorbajio mentioned this pull request Feb 13, 2024

Remove nodejs import from trie.ts #3280

Merged

yann300 reviewed Feb 13, 2024

View reviewed changes

holgerd77 mentioned this pull request Feb 15, 2024

Trie: Analyze readable-steam Dependency Removal/Replacement Options #3218

Closed

scorbajio mentioned this pull request Jul 15, 2024

Replace stream usage with async iterators & pipelines #3507

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Readable stream analysis refactor #3231

Readable stream analysis refactor #3231

scorbajio commented Jan 12, 2024

codecov bot commented Jan 12, 2024 •

edited

Loading

holgerd77 left a comment

holgerd77 Jan 15, 2024

scorbajio Jan 17, 2024

holgerd77 Jan 17, 2024

holgerd77 Jan 15, 2024

jochem-brouwer Jan 16, 2024

holgerd77 Jan 16, 2024

scorbajio Jan 17, 2024

holgerd77 Jan 17, 2024

holgerd77 Jan 17, 2024

yann300 Feb 12, 2024

holgerd77 Feb 12, 2024

scorbajio Feb 13, 2024

scorbajio Feb 14, 2024

holgerd77 left a comment

yann300 Feb 13, 2024

		@@ -1,3 +1,5 @@
		// eslint-disable-next-line implicit-dependencies/no-implicit
		import { ReadableStream } from 'node:stream/web'

Readable stream analysis refactor #3231

Readable stream analysis refactor #3231

Conversation

scorbajio commented Jan 12, 2024

codecov bot commented Jan 12, 2024 • edited Loading

Codecov Report

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 12, 2024 •

edited

Loading