fs: Remove nullable markers from WriteParams members

[usamanadeemdeveloper]

size, position, and data in the WriteParams dictionary were
marked as nullable (?), but the write() algorithm only checks for their
existence and never handles null values. Making them non-nullable means
that passing null will now throw a TypeError at the WebIDL boundary,
which is the correct behavior.

Fixes #181.

This is a spec alignment fix. The algorithm has never handled null for
these members, so making them non-nullable correctly reflects actual
intended behavior (also consistent with Chromium, Gecko, and WebKit
implementations).

• At least two implementers are interested (and none opposed):
  • N/A — this is a spec alignment fix, not a new feature
• Tests are written and can
  be reviewed and commented upon at:
  • fs: add tests for null WriteParams members throwing TypeError web-platform-tests/wpt#58156
• Implementation bugs are filed:
  • Chromium: https://issues.chromium.org/issues/490640057
  • Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=2021767
  • WebKit: https://bugs.webkit.org/show_bug.cgi?id=309446
• MDN issue is filed: N/A
• The top of this comment includes a clear commit
  message to use. ✓

[annevk]

The commit message doesn't seem accurate. You're making them non-nullable, which means that passing null will throw. That is probably what we want, but it will require test changes and such so you need to restore the PR template as well and fill it out.

[usamanadeemdeveloper]

The commit message doesn't seem accurate. You're making them non-nullable, which means that passing null will throw. That is probably what we want, but it will require test changes and such so you need to restore the PR template as well and fill it out.

Thank you for the feedback! I've addressed all three points:

1. Commit message updated to accurately reflect that making these members
   non-nullable means passing null will now throw a TypeError at the WebIDL
   boundary.
2. PR description restored and filled out the PR template.
3. Tests added two missing WPT tests for position: null and size: null
   throwing TypeError in web-platform-tests/wpt (a test for data: null already
   existed). PR: https://github.com/web-platform-tests/wpt/pull/58156

[usamanadeemdeveloper]

The commit message doesn't seem accurate. You're making them non-nullable, which means that passing null will throw. That is probably what we want, but it will require test changes and such so you need to restore the PR template as well and fill it out.

its been a while and no reply from your side would you consider the PR?

[annevk]

I think it should be considered, yes. Your test PR is a 404 however (once you fix that I recommend linking it from the original post at the top, so all the relevant information is collected there). I also want @a-sully or @mkruisselbrink to look at this as this would require changes to all implementations. Which also means you can't say N/A for implementation bugs. Maybe @jesup too.

[usamanadeemdeveloper]

I think it should be considered, yes. Your test PR is a 404 however (once you fix that I recommend linking it from the original post at the top, so all the relevant information is collected there). I also want @a-sully or @mkruisselbrink to look at this as this would require changes to all implementations. Which also means you can't say N/A for implementation bugs. Maybe @jesup too.

AAh I think I did pretty much everything what you asked now! Thanks for guidance

[usamanadeemdeveloper]

Hi @annevk — wanted to give you a full status update and flag a couple of things that need attention.

What's been completed since your last review (Mar 6):

• ✅ WPT tests PR is live: web-platform-tests/wpt#58156 — adds the missing null tests for position: null and size: null throwing TypeError
• ✅ Implementation bugs filed in all three engines:
  • Chromium: https://issues.chromium.org/issues/490640057
  • Gecko (Firefox): https://bugzilla.mozilla.org/show_bug.cgi?id=2021767
  • WebKit: https://bugs.webkit.org/show_bug.cgi?id=309446
• ✅ Commit message updated to accurately reflect the change
• ✅ PR template fully filled out (linked above)

Why I pushed an empty commit today:

CI was failing because bikeshed v6.1.1 requires Python 3.12+, but the repository's workflow was pinned to Python 3.11. After the upstream workflow was updated (to Python 3.14), I needed to push a new commit to re-trigger CI on this PR. I used an empty commit (git commit --allow-empty) purely to kick off a new CI run — no spec changes were made. The CI is now running but is currently awaiting maintainer approval to execute (as required for PRs from forks).

Request:

Could you please approve the CI workflow run so it can execute? And if possible, could you nudge @a-sully and @mkruisselbrink? They were assigned as reviewers on March 2 — it's been 3 weeks with no response. A quick ping from you would go a long way. Happy to provide any additional context they need.

Thanks for your time and guidance throughout this process!

[usamanadeemdeveloper]

@annevk — one small follow-up on the Participation check. I've already signed the individual agreement at https://participate.whatwg.org, so my entry should be in the participant-data repository with verified: false. Per the editor instructions, the remaining step on your end would be to flip that to verified: true and then re-synchronize using the status link. That should turn the check green.

Thanks again for approving the CI run!

[usamanadeemdeveloper]

Hi @annevk — wanted to give you a full status update and flag a couple of things that need attention.

What's been completed since your last review (Mar 6):

• ✅ WPT tests PR is live: web-platform-tests/wpt#58156 — adds the missing null tests for position: null and size: null throwing TypeError

• ✅ Implementation bugs filed in all three engines:

  • Chromium: https://issues.chromium.org/issues/490640057
  • Gecko (Firefox): https://bugzilla.mozilla.org/show_bug.cgi?id=2021767
  • WebKit: https://bugs.webkit.org/show_bug.cgi?id=309446

• ✅ Commit message updated to accurately reflect the change

• ✅ PR template fully filled out (linked above)

Why I pushed an empty commit today:

CI was failing because bikeshed v6.1.1 requires Python 3.12+, but the repository's workflow was pinned to Python 3.11. After the upstream workflow was updated (to Python 3.14), I needed to push a new commit to re-trigger CI on this PR. I used an empty commit (git commit --allow-empty) purely to kick off a new CI run — no spec changes were made. The CI is now running but is currently awaiting maintainer approval to execute (as required for PRs from forks).

Request:

Could you please approve the CI workflow run so it can execute? And if possible, could you nudge @a-sully and @mkruisselbrink? They were assigned as reviewers on March 2 — it's been 3 weeks with no response. A quick ping from you would go a long way. Happy to provide any additional context they need.

Thanks for your time and guidance throughout this process!

Hi @annevk, thanks for the thumbs up! Any idea when @a-sully or @mkruisselbrink might get a chance to review this?

[usamanadeemdeveloper]

I’m attaching this to indicate that Chrome requires the PR to be merged first.

[noamr]

@fergald can someone from your team take a look please?

[mkruisselbrink]

This seems like a reasonable change, and is certainly what the spec should have said from the beginning, my mistake.
There is of course a slight web-compat risk with a change like this as @saschanaz was alluding to in #181. So while this makes sense from a spec point of view, it would require implementers to agree to take on the risk of making the change as well, and I'm no longer involved in the implementation of any of this. Hence hopefully @fergald and other implementers can hopefully weigh in.

[saschanaz]

If we are all agreed on then I'm fine with it too. Maybe would be nice to check known usages like in emscripten. cc @jjjalkanen

[fergald]

IIUC, Chromium currently throws a SyntaxError in this case any of these are null. So the compat risk seems quite low for us.

@mingyc

[annevk]

@fergald isn't that throwing conditional upon the value of type? Because otherwise we might as well make these fields required.

[usamanadeemdeveloper]

I tested this in Chromium and the behavior appears to depend on the execution path rather than being uniform.

In particular:

• When type is present (e.g. "write" / "truncate"), passing null for fields like position, size, or data results in failure during internal algorithm validation, producing a SyntaxError.
• When type is omitted, the failure occurs earlier at the WebIDL boundary with a TypeError due to the required type member being undefined.

So the exception is conditional on the presence of type, but not in a single consistent rule across all paths. Instead, different validation layers are triggered depending on how the dictionary is shaped.

Firefox and Safari do not yet expose showSaveFilePicker, so this was only verifiable in Chromium. I’ve filed the corresponding implementation issues for Gecko and WebKit.

Screenshot of the test results for reference:

This suggests a split between WebIDL-level validation and algorithm-level validation depending on execution path rather than a single unified null-handling rule.

[annevk]

If you always get an exception it's fine, but just from a brief scan of the Chromium code @fergald pointed to I don't think you get an exception when type is "truncate" and position or data is null. Now perhaps that is unlikely enough to not matter in practice, but that would be a new case where we start throwing assuming I'm understanding that correctly from the limited context.

[usamanadeemdeveloper]

If you always get an exception it's fine, but just from a brief scan of the Chromium code @fergald pointed to I don't think you get an exception when type is "truncate" and position or data is null. Now perhaps that is unlikely enough to not matter in practice, but that would be a new case where we start throwing assuming I'm understanding that correctly from the limited context.

Thank you for the precise pushback. I've retested with isolated writables (fresh stream per test) to avoid state contamination from earlier failures.
Results in Chromium:

{ type: "truncate", position: null } → SyntaxError: truncate requires a size argument
{ type: "truncate", data: null } → SyntaxError: truncate requires a size argument

So Chromium does throw in both cases, but the error is driven by the missing/null size, not by position or data being null those fields appear to be ignored under truncate. The exception fires regardless of what else is in the dict.
The remaining compat risk is { type: "write", position: null } which currently passes silently in Chromium. Making position non-nullable would change that to a TypeError at the WebIDL boundary. That is the one case worth implementers explicitly signing off on.

[usamanadeemdeveloper]

@annevk based on the test results, the one genuine behavior change this PR introduces is { type: "write", position: null } which currently passes silently in Chromium. Should I update the PR description to explicitly call this out rather than saying "consistent with implementations"? Happy to clarify the wording if that helps move things forward.

[annevk]

It's not that simple as { type: "truncate", size, position: null } doesn't throw today either. I'm not really familiar with how people use this API though and whether that's likely to happen in practice.

[fergald]

So this would force people to supply a position for truncate and a size for seek? I don't think that's OK.

This would also cause the examples on MDN to start throwing.

Unless I'm misunderstanding things, these have to be optional.

[annevk]

They remain optional. But if you pass them instead of omitting them and use null as value you would get an exception. That means that { type: "truncate", size, position: null } would start throwing, but { type: "truncate", size, position: undefined } would continue not to throw.

(Unfortunately ? is very attractive and browsers binding implementations are often lenient (we fixed this in WebKit which is why this came up) so people don't realize they could (and should) just rely on undefined when designing APIs.)

[fergald]

@annevk thanks. Off-topic but what do you mean by lenient? Looking at Chromium, it seems that both null and undefined are indistinguishable by the time they pass through the bindings and reach our native code. Is that what you mean?

I'm a little nervous about this change. I would like have some data on whether/how often people are making calls with null, however that is not easy given Chromium's bindings. Is Chromium the only implementer?

The practical benefits of this are minimal, so don't see a great argument for taking a risk.

[annevk]

Is that what you mean?

Yes, that's no longer the case in WebKit. In WebKit you are now required to use std::optional<std::optional<T>> for cases like this.

WebKit has this dictionary as well, but we don't have telemetry.