Skip to content

Conversation

@ThatOneBro
Copy link
Contributor

Fixes #1797 and adds a few tests

@Jarred-Sumner
Copy link
Collaborator

Can you add a test that checks if it works when using export from and export * from? Wondering what the compiler will do

@ThatOneBro
Copy link
Contributor Author

ThatOneBro commented Jan 15, 2023

Can you add a test that checks if it works when using export from and export * from? Wondering what the compiler will do

Ah you're right, compiler doesn't like this. Is there any workaround that you can think of?

SyntaxError: Import named 'ReadStream' not found in module '/Users/derrick/repos/bun-stuff/new/bun/test/fixtures/export-lazy-fs-streams/export-*-from.ts'.

and

SyntaxError: Indirectly exported binding name 'ReadStream' is not found.

@ThatOneBro
Copy link
Contributor Author

Ok maybe this isn't worth it, but we could potentially create stub class functions that when constructed lazy load the real ReadStream or WriteStream, so the classes can be loaded without node:stream initially, even in the case where they are re-exported. It would fail an instanceof check, but maybe we can do something clever like the trick used for constructors without new.

Does that make sense at all?

@ThatOneBro ThatOneBro force-pushed the derrick/fix-fs-stream-export branch from 32dc258 to 20f4e8c Compare January 15, 2023 22:54
@ThatOneBro
Copy link
Contributor Author

Oops, just realized I made a noob mistake with the Symbol.hasInstance comparison lol

@ThatOneBro ThatOneBro changed the title fix(node:fs): export fs.ReadStream and fs.WriteStream [WIP] fix(node:fs): export fs.ReadStream and fs.WriteStream Jan 18, 2023
@ThatOneBro ThatOneBro removed the request for review from Jarred-Sumner January 18, 2023 03:19
@ThatOneBro
Copy link
Contributor Author

ThatOneBro commented Jan 18, 2023

Update: fs.ReadStream !== reexportedReadStream // result: true when it should be false. Need another solution, possibly make every instance of fs.ReadStream a wrapper and at the time of construction of the first instance, load node:stream and patch the prototype chain of fs.ReadStream and the instance with the newly loaded NativeReadable subclass. Or we can abandon lazy-loading altogether. But that is costly to startup times as node:stream is very heavy...

@ThatOneBro ThatOneBro force-pushed the derrick/fix-fs-stream-export branch from 4d2a47e to 8a01d67 Compare February 14, 2023 02:36
@ThatOneBro ThatOneBro marked this pull request as draft February 14, 2023 02:48
@Jarred-Sumner Jarred-Sumner marked this pull request as ready for review February 14, 2023 23:54
@Jarred-Sumner Jarred-Sumner merged commit a80981c into main Feb 14, 2023
@Jarred-Sumner Jarred-Sumner deleted the derrick/fix-fs-stream-export branch February 14, 2023 23:54
@Jarred-Sumner
Copy link
Collaborator

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fs.WriteStream is undefined

3 participants