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

Export deepObserve's IChange with more specific name #322

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AndrewADev
Copy link

Adds changes to export the alias of change types supported by deepObserve with a slightly less generic name.

Also adds more detail to the relevant doc comments.

Export

The alias used by MobX-utils for the change parameter of deepObserve, previously named IChange, is not currently exported.

It only consists of types exported by MobX, though. So, when setting up a listener in an application importing mobx-utils, I can define something along these lines to get type information for my listener:

export type IDeepDidChange =
  | IObjectDidChange
  | IArrayDidChange
  | IMapDidChange;

Nonetheless, the exact set of supported types of changes could of course change in the future (such as once #298 addressed). So, exporting this alias directly from MobX-utils would provide up-to-date type information directly from the package itself.

Renaming - Rationale

The thinking for renaming IChange to IDeepDidChange is influenced by:

  • Deep - I thought perhaps it would be good to have something not quite as generic as simply 'change' to reflect the fact that this is coming from MobX-utils, and that there are different types of changes are supported in deepObserve than in MobX core right now (seen in deepObserve not working with ObservableSet #298)
  • What seems to be an existing convention for 'WillChange' vs 'DidChange'

That said, I'm open to a different name than the one proposed, if there is something that better matches MobX conventions or I'm simply reading too much into the conventions here.

Documentation

I'm assuming I should just change the JSDoc comment, as it seems the README is auto-generated before a release? Let me know if that is not the case or I need to update the README as well.

Beyond that, I tried also documenting the callback more thoroughly via a separate JSDoc block, like so:

/**
 * Callback invoked when a deeply observed change is detected.
 *
 * @callback onDeepChange
 * @param {IDeepDidChange} change The type of change
 * @param {string} path The path to the property where the change occurred
 * @param {Object} root The root observed object
 */

Which was valid for generation (I tested with yarn run build-docs), but the callback was then given the same level of nesting as deepObserve (and others) in the README instead of being nested under it (presumably what we'd want). It looks like there is possibly a way to get documentation to do that, though? Let me know if you see value in pursuing that (also for elsewhere)

Export the type alias that contains information on the types of changes currently supported by `deepObserve`.

Also rename it to be more specific and reduce the risk that it is mistaken for something coming from the main MobX library.
Document types and descriptions, extend example.
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.

1 participant