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 CreateObservableOptions type #2717

Merged
merged 3 commits into from
Jan 22, 2021

Conversation

ahoisl
Copy link
Contributor

@ahoisl ahoisl commented Jan 15, 2021

Code change checklist

  • IMO unnecessary - Added/updated unit tests
  • IMO unnecessary - Updated /docs. For new functionality, at least API.md should be updated
  • Verified that there is no significant performance drop (npm run perf)

Fixes issue #2716

@changeset-bot
Copy link

changeset-bot bot commented Jan 15, 2021

🦋 Changeset detected

Latest commit: d47b5f3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
mobx Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@danielkcz
Copy link
Contributor

Thanks for your contribution. Please add the changeset as per the comment above.

@urugator
Copy link
Collaborator

CreateObservableOptions defines undocumented fields that shouldn't be relied on.

@ahoisl
Copy link
Contributor Author

ahoisl commented Jan 17, 2021

CreateObservableOptions defines undocumented fields that shouldn't be relied on.

Sorry, I don't really understand. The options are documented here, so what do you mean by "undocumented"?
https://mobx.js.org/observable-state.html#options-

These are the default options used by makeObservable - are we talking about different things?

@urugator
Copy link
Collaborator

export type CreateObservableOptions = {
name?: string
equals?: IEqualsComparer<any>
deep?: boolean
defaultDecorator?: Annotation
proxy?: boolean
autoBind?: boolean
}

defaultDecorator, equals, proxy shouldn't be part of the public API

@ahoisl
Copy link
Contributor Author

ahoisl commented Jan 17, 2021

@urugator That's a different problem then in my opinion, because these options already are part of the public API. If you use makeObservable (or makeAutoObservable), the will already show up as valid options. We probably need a sub-type then? That would be a breaking change to the API, though, to my understanding.

@urugator
Copy link
Collaborator

@mweststrate what's the intention/reasoning here?
equals is only used by observable.box ...
proxy seems to be used only by (2) tests (to force non-proxy impl)
defaultDecorator doesn't seem to be used at all, perhaps backward compatibility?

@mweststrate
Copy link
Member

@urugator equals and defaultDecorator are internally be used by .struct, .ref and .shallow? If not I think it has been accidentally left there by a past cleanup. proxy should definitinely be documented and is an omission of mine in https://mobx.js.org/observable-state.html#options-; it is pretty useful as it allows you to skip using Proxies if the object shape is stable, which is (probably) faster, compatible with IE and a lot debug friendlier (it is still a shame debuggers don't treat proxies nicer).

@mweststrate
Copy link
Member

@ahoisl ahoisl force-pushed the ahoisl-export-CreateObservableOptions branch from ee35503 to f0d02b7 Compare January 18, 2021 20:59
mweststrate added a commit that referenced this pull request Jan 18, 2021
@ahoisl ahoisl force-pushed the ahoisl-export-CreateObservableOptions branch from f0d02b7 to ccfa589 Compare January 20, 2021 13:12
@ahoisl
Copy link
Contributor Author

ahoisl commented Jan 20, 2021

@urugator I don't think I can do much more for the issue with the probably "left-over" props in the options equals and defaultDecorator. In any way, people could have already used those options and as I understand it, they would have an impact for some cases. So changing or removing the props would be a breaking change.

As these props are already public and exporting the CreateObservableOptions does not make a difference in regards to these props already being public/exported, can we close this PR and handle the props in a separate issue (if necessary)?

@mweststrate
Copy link
Member

I think it is fine to merge this

@ahoisl ahoisl force-pushed the ahoisl-export-CreateObservableOptions branch from ccfa589 to 82509b7 Compare January 22, 2021 08:38
@danielkcz danielkcz merged commit 65c7b73 into mobxjs:main Jan 22, 2021
@github-actions github-actions bot mentioned this pull request Jan 22, 2021
@ahoisl ahoisl deleted the ahoisl-export-CreateObservableOptions branch January 22, 2021 10:07
mweststrate added a commit that referenced this pull request Jan 23, 2021
* docs: Document enumerability as discussed in #2641

* docs: Document `proxy` option as discussed in #2717

* docs: use absolute urls in Readme, fixes #2685

* Add changeset

* Name fix
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.

5 participants