Skip to content

refactor: extract Document, Animation, and XPath interfaces - #287

Open
jderochervlk wants to merge 6 commits into
refactor-locationfrom
refactor-document
Open

refactor: extract Document, Animation, and XPath interfaces#287
jderochervlk wants to merge 6 commits into
refactor-locationfrom
refactor-document

Conversation

@jderochervlk

@jderochervlk jderochervlk commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Tracking issue: #342

Summary

  • move Document methods and supporting option records into the owning Document module
  • define Animation.t, animation state types, and animation methods locally
  • define XPath expression and result types in their owning modules
  • remove the corresponding broad aliases from DomTypes

Temporary state

Review focus

  • the extracted Document, Animation, and XPath signatures
  • public API continuity while removing their DomTypes aliases

Verification

  • npm run build
  • npm test
  • npm run format:check

@jderochervlk jderochervlk changed the title refactor Document and related files refactor Document, element, and related files Jul 8, 2026
@jderochervlk
jderochervlk marked this pull request as draft July 8, 2026 19:29
@jderochervlk
jderochervlk marked this pull request as ready for review July 8, 2026 21:26

@brnrdog brnrdog left a comment

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.

I liked the types living within their interfaces, makes them much more discoverable than before with the DOMTypes. I think the DOMTree is fair given the recursive types.

I'm just missing a .t for Element/Node/HTMLElement, like we do for other modules. What if we add a small alias on each like:

type t = DOMTree.element
include Impl({type t = t})

@jderochervlk

Copy link
Copy Markdown
Collaborator Author

I liked the types living within their interfaces, makes them much more discoverable than before with the DOMTypes. I think the DOMTree is fair given the recursive types.

I'm just missing a .t for Element/Node/HTMLElement, like we do for other modules. What if we add a small alias on each like:

type t = DOMTree.element
include Impl({type t = t})

Yeah, I think having a type t on modules makes sense and I do like it. I'll make sure those get added in.

@jderochervlk jderochervlk changed the title refactor Document, element, and related files refactor: extract Document and related interfaces Aug 3, 2026
@jderochervlk jderochervlk changed the title refactor: extract Document and related interfaces refactor: extract Document, Animation, and XPath interfaces Aug 3, 2026
@jderochervlk
jderochervlk requested a review from brnrdog August 3, 2026 19:49
Comment thread src/Base/DOM.res Outdated

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: 2e9ab9f34c

ℹ️ 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/Animation.res
Comment thread src/DOM/Document.res Outdated
@jderochervlk
jderochervlk force-pushed the refactor-document branch 2 times, most recently from 9d52633 to 4de88f7 Compare August 5, 2026 12:14
@jderochervlk
jderochervlk force-pushed the refactor-document branch 2 times, most recently from 7283c5e to b7ad5ca Compare August 6, 2026 14:52
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.

3 participants