Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify base changeListener. #29

Merged
merged 1 commit into from Feb 5, 2015
Merged

Conversation

dashed
Copy link
Contributor

@dashed dashed commented Feb 5, 2015

When handling resync of stale cursors with the underlying immutable structure (e.g map),
we need to handle three cases based on the given keyPath indicating a change:

  1. keyPath is not in the updated map, but it exists in the current (removal)
  2. keyPath is in the updated map, but it doesn't exist in the current (add)
  3. keyPath is in the updated map, but it also exists in the current (update)

For the first case, we need to use Immutable.Map.removeIn.
While Immutable.Map.setIn is sufficient for the other cases.

Using methods of simplifying boolean expressions (e.g. K-maps), we can just test
keyPath is not in the updated map to handle logic flow.

When handling resync of stale cursors with the underlying immutable structure (e.g map),
we need to handle three cases based on the given keyPath indicating a change:

1. keyPath is not in the updated map, but it exists in the current (removal)
2. keyPath is in the updated map, but it doesn't exist in the current (add)
3. keyPath is in the updated map, but it also exists in the current

For the first case, we need to use `Immutable.Map.removeIn`.
While `Immutable.Map.setIn` is sufficient for the other cases.

Using methods of simplifying boolean expressions (e.g. K-maps), we can just test
keyPath is not in the updated map to handle logic flow.
@dashed
Copy link
Contributor Author

dashed commented Feb 5, 2015

Thanks to @Gozala for bringing this up.

@dashed
Copy link
Contributor Author

dashed commented Feb 5, 2015

This relies on knowing the following: https://github.com/facebook/immutable-js/blob/691ad67eff57d2addd739848c195140e6f488f94/src/Map.js#L58-L60

Correctness should be the same overall.

@torgeir
Copy link
Member

torgeir commented Feb 5, 2015

Looks good to me!

mikaelbr added a commit that referenced this pull request Feb 5, 2015
Simplifies updating structure on `changeListener`.
@mikaelbr mikaelbr merged commit b908882 into omniscientjs:master Feb 5, 2015
@mikaelbr
Copy link
Member

mikaelbr commented Feb 5, 2015

Thanks! Looks good. Less code is great.

@dashed dashed deleted the simplify branch February 5, 2015 10:33
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.

None yet

3 participants