Skip to content

Loading…

Improve notifyPath/notifySplices API for public usage #2509

Closed
kevinpschaaf opened this Issue · 4 comments

2 participants

@kevinpschaaf

The current path notification API has a couple of warts making it problematic for use by data adapter layers providing alternate forms of data observation:

  • notifyPath accepts only collection keys for arrays (as opposed to array indicies as set does). With the changes to resolve #2007, set will accept either keys or indicies, and the public version of notifyPath should do so as well; this will likely involve refactoring internal usage of notifyPath to avoid the overhead of key/index disambiguation when it is known that the path is key-based
  • _notifySplices is currently private and undocumented; for a fully operable data observation layer, this needs to be exposed publicly
@kevinpschaaf kevinpschaaf added the p1 label
@kevinpschaaf kevinpschaaf self-assigned this
@kevinpschaaf kevinpschaaf referenced this issue from a commit
@kevinpschaaf kevinpschaaf Improvements to path API. Fixes #2509.
* Allows `set` to take paths with array #keys
* Allows `notifyPath` to take paths with array indices
* Exposes public notifySplices API
3c25d25
@kevinpschaaf
Owner

PR here: #2513

@kevinpschaaf kevinpschaaf closed this issue from a commit
@kevinpschaaf kevinpschaaf Improvements to path API. Fixes #2509.
* Allows `set` to take paths with array #keys
* Allows `notifyPath` to take paths with array indices
* Exposes public notifySplices API
10021cc
@devinivy

@kevinpschaaf I think the docs may need some adjustment now, given that set no longer leverages notifyPath for sugar. Thinking of this quote from the primer,

Since in the majority of cases, notifyPath will be called directly after an assignment, a convenience function set is provided that performs both the assignment and notify actions.

I realize that it doesn't explicitly say that set uses notifyPath directly, but it seems to imply that this is the case. Now set uses the private interface _notifyPath.

Out of curiosity, is there any way set, splice, push, etc. could continue to use the notifyPath public interface? We were patching notifyPath to add functionality to set, splice, push, etc. It's a nice assumption to be able to make, that notifyPath is used by all those methods.

@kevinpschaaf

That the other API's now use _notifyPath rather than the public notifyPath is an implementations detail. We do this for performance reasons: since the public notifyPath has to do the index->key normalization, we are careful to only do that once in the other public API's, and call _notifyPath internally, which accepts only pre-normalized key-based paths. So I don't think we'd consider changing that back as it's a performance consideration.

@devinivy

Cool– that makes sense, esp given that this code is hot. Thanks for getting back!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.