Skip to content

fix(webapi): correct binding nullability and exception contracts [Codex] - #349

Open
jderochervlk wants to merge 5 commits into
mainfrom
agent/audit-webapi-bindings
Open

fix(webapi): correct binding nullability and exception contracts [Codex]#349
jderochervlk wants to merge 5 commits into
mainfrom
agent/audit-webapi-bindings

Conversation

@jderochervlk

@jderochervlk jderochervlk commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Corrects nullability and optionality across affected Web API bindings.
  • Adds @throws(JsExn) where synchronous browser APIs can raise exceptions.
  • Models HTMLElement.hidden with the semantic states Visible, Hidden, and HiddenUntilFound, without a raw coercion API.
  • Updates compile-time fixtures for the corrected contracts.

HTMLElement.hidden

The getter and setter use the same semantic type:

element->HTMLElement.setHidden(HTMLElement.Hidden)
element->HTMLElement.setHidden(HTMLElement.HiddenUntilFound)
element->HTMLElement.setHidden(HTMLElement.Visible)

switch element->HTMLElement.hidden {
| HTMLElement.Visible => "visible"
| HTMLElement.Hidden => "hidden"
| HTMLElement.HiddenUntilFound => "hidden until found"
}

Because setHidden is an @set external, these calls emit direct property assignments such as element.hidden = true.

An earlier version also exposed hiddenInput and setHiddenRaw to mirror every Web IDL setter input. Those were removed because numbers, arbitrary strings, null, and undefined only invoke coercion and do not represent additional states. The binding intentionally restricts writes to the three useful outcomes. See MDN and the HTML Standard.

@jderochervlk

Copy link
Copy Markdown
Collaborator Author

@codex

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 29, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-03T13:26:07.202219Z bbe9c53 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a11e7cfb79

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/dom-nodes/DOMTree.res Outdated
@jderochervlk jderochervlk changed the title fix: audit Web API binding contracts fix: correct Web API nullability and exception contracts Sep 2, 2026
@jderochervlk jderochervlk changed the title fix: correct Web API nullability and exception contracts fix(webapi): correct binding nullability and exception contracts [Codex] Sep 3, 2026
@jderochervlk

Copy link
Copy Markdown
Collaborator Author

@codex

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bbe9c53735

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/html/HTMLElement.res Outdated
@jderochervlk
jderochervlk marked this pull request as ready for review September 3, 2026 13:53
@fhammerschmidt

Copy link
Copy Markdown
Member

That's a bit unfortunate. Is
element.hidden = true
now not working anymore or just not the official way to do it?

The other stuff is great. Makes me want to first-class the throws annotation into the compiler itself to actually always know when a JS function throws an exception.

jderochervlk commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

Here's the explanation from Codex:

Yes, element.hidden = true no longer type-checks against the updated bindings. This is a ReScript API change, not a browser runtime change:

element->HTMLElement.setHidden(HTMLElement.Hidden)

setHidden is an @set external, so that call still emits a native element.hidden = true assignment. It does not add a JavaScript method or wrapper.

A variant handles the semantic states, and that is what hiddenState does. It cannot fully model the property as one mutable field because the getter and setter have different contracts: reads normalize to false, true, or "until-found", while writes also accept numbers, arbitrary strings, null, and undefined. A mutable record field would have to use one type for both directions, either widening reads to impossible states or narrowing valid writes. The typed getter and setHidden cover normal use; setHiddenRaw preserves the wider setter contract.

I added this explanation to the PR description: Hidden property.

@jderochervlk

Copy link
Copy Markdown
Collaborator Author

Updated this after exploring the alternatives. element.hidden = true still does not type-check with the new bindings; the typed equivalent is:

element->HTMLElement.setHidden(HTMLElement.Hidden)

Because setHidden is an @set external, this emits the same element.hidden = true property assignment.

We have now removed hiddenInput and setHiddenRaw. Although the native setter accepts booleans, numbers, strings, null, and undefined, those inputs are coercion paths to the same three observable states: visible, hidden, or hidden-until-found. Exposing separate raw or type-specific functions would add API surface without adding a useful state, so the binding now accepts only hiddenState.

The updated rationale and usage example are in the HTMLElement.hidden section of the PR description.

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