Skip to content

refactor: extract Location bindings - #286

Open
jderochervlk wants to merge 4 commits into
mainfrom
refactor-location
Open

refactor: extract Location bindings#286
jderochervlk wants to merge 4 commits into
mainfrom
refactor-location

Conversation

@jderochervlk

@jderochervlk jderochervlk commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Tracking issue: #342

Summary

  • define Location.t and the Location bindings in the owning Location module
  • remove the duplicated location record from DOM and DomTypes
  • return Location.t from Document.location and Window.location
  • add the Location.from and Location.fromDocument conversion bindings

Temporary state

  • this extraction does not introduce a Location compatibility alias; Location.t is the durable public type
  • Document, Window, and the remaining broad DomTypes definitions are migrated by later PRs, ending with deletion of DomTypes in refactor: remove DOM compatibility types #310
  • Location.res remains physically under the current broad DOM source folder until the follow-up Option 5 folder/feature stack assigns final folder ownership

Review focus

  • completeness of the extracted Location record and bindings
  • preservation of the Document.location and Window.location contracts

Verification

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

@tsnobip

tsnobip commented Jul 9, 2026

Copy link
Copy Markdown
Member

@jderochervlk same comment as in #288, what's the motivation here? I think the main goal of our work now should be to have the most common types inside the Base feature so dependent packages like @rescript/react-dom remain light and it looks like you're going in the opposite direction. I'd do the other way around and see what's needed to have @rescript/react-dom depend only on Base feature. Secondary goals could be to set up features' dependencies and to make the DOM feature less bloated (but I'm not sure this is something we could do while sticking to the same structure as on MDN).

@jderochervlk

jderochervlk commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

@jderochervlk same comment as in #288, what's the motivation here? I think the main goal of our work now should be to have the most common types inside the Base feature so dependent packages like @rescript/react-dom remain light and it looks like you're going in the opposite direction. I'd do the other way around and see what's needed to have @rescript/react-dom depend only on Base feature. Secondary goals could be to set up features' dependencies and to make the DOM feature less bloated (but I'm not sure this is something we could do while sticking to the same structure as on MDN).

rescript-react does not depend on Location, so by moving this out it removes that from DOM.

I'm trying to slim down DOM.res as much as possible, you can see a larger lift in that direction on this PR: #287

I don't think we'll need to keep the Base feature and can just have DOM serve as the base feature once the file is small enough.

@brnrdog

brnrdog commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

Makes sense to me from the code and API perspective. Moving Location and reserving the Base for the types most things actually depend on feels like the right call.

I do agree with @tsnobip's point, consumers like @rescript/react need to depend on Base alone and stay light. But we can still go with these changes, makes it more composable and lightweight.

@jderochervlk jderochervlk changed the title refactor Location refactor: extract Location bindings Aug 3, 2026
@jderochervlk
jderochervlk changed the base branch from codex/event-api-split to codex/dom-window-access August 3, 2026 18:39
@jderochervlk
jderochervlk requested review from brnrdog and tsnobip August 3, 2026 19:48

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

ℹ️ 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/Location.res Outdated
Comment thread src/DOM/Location.res Outdated
@jderochervlk
jderochervlk changed the base branch from codex/dom-window-access to codex/event-api-split August 4, 2026 17:30
@jderochervlk
jderochervlk force-pushed the refactor-location branch 3 times, most recently from e57c513 to f654113 Compare August 5, 2026 15:08
Comment thread src/Base/DOM.res
Comment thread src/DOM/Document.res Outdated
Base automatically changed from codex/event-api-split to main August 6, 2026 14:57
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