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.

Sign up for GitHub

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

Jump to bottom

The value returned from computed(() => ...) shouldn't have a set(...) method #3767

Open
Nokel81 opened this issue Sep 26, 2023 · 8 comments · May be fixed by #3783
Open

The value returned from computed(() => ...) shouldn't have a set(...) method #3767

Nokel81 opened this issue Sep 26, 2023 · 8 comments · May be fixed by #3783
Labels
🐛 bug

Comments

@Nokel81
Copy link
Contributor

Nokel81 commented Sep 26, 2023 •
edited
Loading

Intended outcome:

Have strict and complete type safety to help with refactoring.

Actual outcome:

The return type of computed() and observable.box() both have set() and get() methods, but the return type of computed() doesn't have a set() method so a type error is thrown at run time.

How to reproduce the issue:

const myObservableValue = observable.box(10);
const myComputedValue = computed(() => myObservableValue.get() + 1);

/**
 * Uncaught:
 * Error: [MobX] [ComputedValue 'ComputedValue@2'] It is not possible to assign a new value to a computed value.
 *    at die (/node_modules/mobx/dist/mobx.cjs.development.js:89:11)
 *    at ComputedValue.set (/node_modules/mobx/dist/mobx.cjs.development.js:1448:7)
 */
myComputedValue.set(12);

Versions

mobx -> 6.10.9
@mweststrate
Copy link
Member

mweststrate commented Sep 27, 2023 via email

@Nokel81
Copy link
Contributor Author

Nokel81 commented Sep 27, 2023

Would you like me to recreate this filling then?

@mweststrate
Copy link
Member

yes, or copy in the template.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Sep 27, 2023

done

@mweststrate
Copy link
Member

mweststrate commented Sep 27, 2023 •
edited
Loading

The above example is working as expected if no custom setter is defined. There is a .set method, and you are able to call it successfully, it just throws because the preconditions aren't met, not because the method doesn't exist.

N.b. note that in general in an issue report a reproduction is something "runnable". For further follow up questions make sure to attach a codesandbox or something.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Sep 27, 2023

Sure, but won't it be better for computed(() => someObservable.get()) to not even allow .set() to be called via typescript?

@mweststrate
Copy link
Member

Well, '.set' is a valid API of computed, that the runtime requirements aren't met isn't of typescripts concern. (could do some complicated juggling to make that detectable in TS in cases, but doubt that'd be worth the effort)

@Nokel81
Copy link
Contributor Author

Nokel81 commented Sep 27, 2023

It is actually rather easy, see my playground here

@Nokel81 Nokel81 linked a pull request Oct 26, 2023 that will close this issue
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
🐛 bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix!: Prevent calling set on the return value of 'computed()' Nokel81/mobx
2 participants
@mweststrate @Nokel81