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

add warn for extending builtins #2769

Merged
merged 4 commits into from
Feb 4, 2021

Conversation

iChenLei
Copy link
Member

Code change checklist

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

PR Background

issue -> fix #2765
Add friendly warn for extending builtins action.
Current 👇
2021-01-31 11 48 36
After 👇
2021-01-31 11 56 11

@changeset-bot
Copy link

changeset-bot bot commented Jan 31, 2021

🦋 Changeset detected

Latest commit: 781de7a

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

@urugator
Copy link
Collaborator

urugator commented Jan 31, 2021

Thanks for the PR, but please try to handle it like this:

if (hasProp(target, $mobx)) return target

if (hasProp(target, $mobx)) {
  if (__DEV__ && !(getAdministration(target) instanceof ObservableObjectAdministration)) {
     // Something like
     die(`Cannot convert '${getDebugName(target)}' into observable object: The target is already observable of different type. Extending builtins is not supported.`)
  }
  return target
}

That should cover other unsupported cases like makeAutoObservable/extendObservable(boxedObservable|observableArray|observableMap|observableSet) etc
And please add tests for these other types.
EDITED

@iChenLei
Copy link
Member Author

@urugator Yes, sir. 😃

@iChenLei
Copy link
Member Author

iChenLei commented Jan 31, 2021

@urugator Hi, sir. Did we need test all types? I found that only observableMap and observableSet are export from mobx npm package.

packages/mobx/src/mobx.ts

extendObservable

https://mobx.js.org/api.html#extendobservable

2021-02-01 00 28 36

May be it's only use for function/object ? Any suggestion to test it ?

makeAutoObservable

// https://github.com/mobxjs/mobx/blob/main/packages/mobx/src/api/makeObservable.ts#L50-L55
if (__DEV__) {
    if (!isPlainObject(target) && !isPlainObject(Object.getPrototypeOf(target)))
        👇  // user can't extending builtin, so don't need check more
        die(`'makeAutoObservable' can only be used for classes that don't have a superclass`) 
    if (isObservableObject(target))
        die(`makeAutoObservable can only be used on objects not already made observable`)
}

@urugator
Copy link
Collaborator

urugator commented Jan 31, 2021

Did we need test all types?

It's not about preventing extend Builtin.
It's about inability to pass observable of different type than object to extendObservable/makeObservable/makeAutoObservable.
So makeObservable(observable([])), extendObservable(observable.box()) etc should fail (not neccessarily with the same error, but they must not pass).
It doesn't have to happen inside constructor when extending, thats just a special case.
Is it clear?

Copy link
Member

@mweststrate mweststrate left a comment

Choose a reason for hiding this comment

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

Looking good, thanks for making this change!

@mweststrate mweststrate merged commit 7820e5e into mobxjs:main Feb 4, 2021
@github-actions github-actions bot mentioned this pull request Feb 4, 2021
@iChenLei iChenLei deleted the warn-extends-builtin branch February 4, 2021 11:36
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.

makeObservable crashing on class derived from ObservableMap for mobx 6.1.x
3 participants