Skip to content

Conversation

@ra1028
Copy link
Owner

@ra1028 ra1028 commented Mar 3, 2025

Pull Request Type

  • Bug fix
  • New feature
  • Refactoring
  • Documentation update
  • Chore

Description

Expected overrides and observers are not reflected when atoms are updated across the scope boundary.
For example, let's say you have AtomA, which depends on AtomB and AtomC. AtomB is scoped due to override, AtomC is shared. Now, AtomC gets updated, and it also updated the downstream - AtomA. AtomA accesses AtomB to calculate its new value, but it fails to get the overridden value because it's updated from the scope of AtomC, which is not intuitive. In terms of API ergonomics, the AtomA should definitely be updated with the scope in which the AtomA was initialized.

@ra1028 ra1028 force-pushed the fix/scoped-overrides-observers-not-reflected branch from 2469a08 to 90b988f Compare March 5, 2025 09:57
Copy link
Contributor

@shingt shingt left a comment

Choose a reason for hiding this comment

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

Performed some testing on the app, too / LGTM. Thank you!!

Comment on lines +5 to +6
private let rootScopeKey: ScopeKey
private let currentScopeKey: ScopeKey?
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense

@ra1028 ra1028 marked this pull request as ready for review March 14, 2025 09:32
@ra1028 ra1028 merged commit 8b583f7 into main Mar 14, 2025
8 checks passed
@ra1028 ra1028 deleted the fix/scoped-overrides-observers-not-reflected branch March 14, 2025 09:32
@ra1028 ra1028 requested a review from Copilot March 14, 2025 09:51
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 35 out of 35 changed files in this pull request and generated no comments.

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