Skip to content

Conversation

@jasnell
Copy link
Collaborator

@jasnell jasnell commented May 12, 2025

Not yet a full implementation but incrementally getting there. This adds impls for most of the fs sync APIs with a compat flag to enable. Wranglers polyfill overrides are still necessary until these are considered fully complete, tested, and optimized. It should be safe to land these as is, even if still incomplete and not fully tested as an experimental compat flag is required to enable them. I'm going to try to work on these over several PRs in order to keep the overall size of each PR down.

@jasnell jasnell requested review from anonrig and danlapid May 12, 2025 21:27
@jasnell jasnell requested review from a team as code owners May 12, 2025 21:27
@jasnell jasnell requested a review from a team May 12, 2025 21:27
@anonrig
Copy link
Member

anonrig commented May 12, 2025

Why didn't you push this on top of #3796? I think we should keep all implementation in a single branch as discussed, before merging.

// eslint-disable-next-line @typescript-eslint/no-explicit-any,@typescript-eslint/no-unsafe-member-access
!(globalThis as any).Cloudflare.compatibilityFlags['enable_node_file_system']
) {
throw new Error('The experimental node:fs implementation is not enabled');
Copy link
Member

Choose a reason for hiding this comment

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

Even though this is behind an experimental flag, we're now throwing a different error message, then we do in the current state. The correct way to do this is through module loader, and just act like "node:fs" doesn't exist (which is the prior behavior to this PR)

@jasnell
Copy link
Collaborator Author

jasnell commented May 12, 2025

Will merge these changes into the other PR.

That said, there's no reason not to merge into main incrementally under the experimental compat flag so will still likely seek to do the implementation incrementally over a series of individual PRs.

@jasnell jasnell closed this May 12, 2025
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