Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace readable-stream usage with Web Streams in the Vaults and Git Domains #523

Closed
1 task done
tegefaulkes opened this issue May 5, 2023 · 7 comments · Fixed by #709
Closed
1 task done

Replace readable-stream usage with Web Streams in the Vaults and Git Domains #523

tegefaulkes opened this issue May 5, 2023 · 7 comments · Fixed by #709
Assignees
Labels
development Standard development r&d:polykey:core activity 1 Secret Vault Sharing and Secret History Management

Comments

@tegefaulkes
Copy link
Contributor

tegefaulkes commented May 5, 2023

Specification

Some parts of the code are still using readable-stream. these need to be replaced with webstreams and the readable-stream dependency removed from the package.json.

These usages can be found at


[nix-shell:~/Projects/Polykey]$ ag "from 'readable"
src/vaults/VaultManager.ts
24:import { PassThrough } from 'readable-stream';

src/git/types.ts
1:import type { PassThrough } from 'readable-stream';

src/git/utils.ts
22:import { PassThrough } from 'readable-stream';

Additional context

Tasks

  1. [ ] 1. Replace any readable-stream usage with stream/web. - replacing with async generator
  2. 2. Remove readable-stream as a dependency from the package.json.
@tegefaulkes tegefaulkes added the development Standard development label May 5, 2023
@tegefaulkes tegefaulkes self-assigned this May 5, 2023
@tegefaulkes tegefaulkes changed the title Replace readable-stream usage with webstreams Replace readable-stream usage with stream/web May 5, 2023
@CMCDragonkai
Copy link
Member

The readable-stream is also being used by EFS. That can't really be changed, it has to simulate node streams. However it's a really old readable stream. Might be better upgraded to match the latest stream behaviour. Not sure.

@CMCDragonkai CMCDragonkai added the r&d:polykey:core activity 1 Secret Vault Sharing and Secret History Management label Jul 9, 2023
@CMCDragonkai
Copy link
Member

I'm putting this under #298 because the usage of readable streams is relevant to git and EFS which is still using the old readable stream implementation in order to simulate how Node streams work, but Node streams have changed quite a bit since the initial import from VFS.

@CMCDragonkai
Copy link
Member

Ok vaults domain should be using web streams, same with git. The git domain needs significant refactoring.

The EFS has to continue using "fs streams", it just requires an update to the fs stream backing library, and then testing to see that everything still works.

1 similar comment
@CMCDragonkai
Copy link
Member

Ok vaults domain should be using web streams, same with git. The git domain needs significant refactoring.

The EFS has to continue using "fs streams", it just requires an update to the fs stream backing library, and then testing to see that everything still works.

@CMCDragonkai CMCDragonkai changed the title Replace readable-stream usage with stream/web Replace readable-stream usage with Web Streams in the Vaults and Git Domains Dec 15, 2023
Copy link
Contributor Author

tegefaulkes commented May 13, 2024

This might actually be addressed by the #709 PR, I'll double check and include this issue as fixed if so.

@CMCDragonkai
Copy link
Member

Yes so if this is done, remove the dependency from package.json too. It will continue to be used by EFS until it can be refactored too.

@tegefaulkes
Copy link
Contributor Author

I double checked and it's all gone. I've removed the dependency in #709 and made it fixed by that as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development r&d:polykey:core activity 1 Secret Vault Sharing and Secret History Management
2 participants