Skip to content

fs: Remove nullable markers from WriteParams members#182

Open
usamanadeemdeveloper wants to merge 2 commits intowhatwg:mainfrom
usamanadeemdeveloper:fix/writeparams-remove-nullable
Open

fs: Remove nullable markers from WriteParams members#182
usamanadeemdeveloper wants to merge 2 commits intowhatwg:mainfrom
usamanadeemdeveloper:fix/writeparams-remove-nullable

Conversation

@usamanadeemdeveloper
Copy link

@usamanadeemdeveloper usamanadeemdeveloper commented Mar 1, 2026

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).

@annevk
Copy link
Member

annevk commented Mar 1, 2026

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.

`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 whatwg#181.
@usamanadeemdeveloper usamanadeemdeveloper force-pushed the fix/writeparams-remove-nullable branch from 98be590 to 1122ace Compare March 1, 2026 18:32
@usamanadeemdeveloper
Copy link
Author

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
Copy link
Author

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
Copy link
Member

annevk commented Mar 6, 2026

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
Copy link
Author

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 usamanadeemdeveloper force-pushed the fix/writeparams-remove-nullable branch from f9b48c8 to 098f00d Compare March 22, 2026 12:03
@usamanadeemdeveloper
Copy link
Author

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):

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
Copy link
Author

@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
Copy link
Author

usamanadeemdeveloper commented Mar 24, 2026

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):

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?

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Should the optional members of WriteParams be nullable?

2 participants