Skip to content

feat(p3-shim): implement wasi preview3 nodejs filesystem API#724

Merged
vados-cosmonic merged 13 commits into
bytecodealliance:mainfrom
andreiltd:p3-filesystem
Jun 5, 2025
Merged

feat(p3-shim): implement wasi preview3 nodejs filesystem API#724
vados-cosmonic merged 13 commits into
bytecodealliance:mainfrom
andreiltd:p3-filesystem

Conversation

@andreiltd
Copy link
Copy Markdown
Member

Split from: #653

Copy link
Copy Markdown
Collaborator

@vados-cosmonic vados-cosmonic left a comment

Choose a reason for hiding this comment

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

Thanks for the hard work here @andreiltd and splitting it off to make it easier to review!

Comment thread .github/workflows/main.yml Outdated
Comment thread packages/preview3-shim/eslint.config.js Outdated
Comment thread packages/preview3-shim/lib/nodejs/filesystem/error.js
Comment thread packages/preview3-shim/lib/nodejs/filesystem/error.js Outdated
Comment thread packages/preview3-shim/lib/nodejs/filesystem/error.js Outdated
Comment thread packages/preview3-shim/lib/nodejs/filesystem/descriptor.js Outdated
Comment thread packages/preview3-shim/lib/nodejs/filesystem/descriptor.js Outdated
Comment thread packages/preview3-shim/lib/nodejs/filesystem/descriptor.js Outdated
Comment thread packages/preview3-shim/lib/nodejs/filesystem/descriptor.js Outdated
Comment thread packages/preview3-shim/lib/nodejs/filesystem/descriptor.js Outdated
@andreiltd andreiltd force-pushed the p3-filesystem branch 3 times, most recently from 7594d29 to 8798626 Compare June 3, 2025 10:57
Copy link
Copy Markdown
Collaborator

@vados-cosmonic vados-cosmonic left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Some minor comments but this should be ready to merge after they're resolved!

Comment thread packages/preview3-shim/lib/nodejs/filesystem/error.js Outdated
Comment thread packages/preview3-shim/lib/nodejs/finalization.js Outdated
Comment thread packages/preview3-shim/lib/nodejs/future.js
@@ -0,0 +1,185 @@
export class StreamReader {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That link isn't working, but I think we should be consistent here -- if you need to expose the reader and writer classes, then that's fine! I value the consistency of them being public or not.

IMO if you'd like to have a constructor that allows for a different implementation of the streams to be sent in, then could be a separate function people can call (or other options to the future()/stream() functions). That said, I'm fine either way here -- let's make them (consistently) public for now, we've bikeshed on this long enough, probably.

@vados-cosmonic vados-cosmonic added this pull request to the merge queue Jun 5, 2025
@vados-cosmonic vados-cosmonic removed this pull request from the merge queue due to a manual request Jun 5, 2025
@vados-cosmonic vados-cosmonic enabled auto-merge June 5, 2025 14:15
auto-merge was automatically disabled June 5, 2025 14:25

Head branch was pushed to by a user without write access

@vados-cosmonic vados-cosmonic changed the title feat: implement wasi preview3 nodejs filesystem API feat(jco): implement wasi preview3 nodejs filesystem API Jun 5, 2025
@vados-cosmonic vados-cosmonic added this pull request to the merge queue Jun 5, 2025
@vados-cosmonic vados-cosmonic removed this pull request from the merge queue due to a manual request Jun 5, 2025
@vados-cosmonic vados-cosmonic added this pull request to the merge queue Jun 5, 2025
@vados-cosmonic vados-cosmonic removed this pull request from the merge queue due to a manual request Jun 5, 2025
@vados-cosmonic vados-cosmonic changed the title feat(jco): implement wasi preview3 nodejs filesystem API feat(p3-shim): implement wasi preview3 nodejs filesystem API Jun 5, 2025
@vados-cosmonic vados-cosmonic added this pull request to the merge queue Jun 5, 2025
Merged via the queue into bytecodealliance:main with commit 096c5e5 Jun 5, 2025
27 checks passed
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.

2 participants