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

Allow overwriting computed values if configured #2011

Merged

Conversation

jillesme
Copy link
Collaborator

@jillesme jillesme commented Jun 18, 2019

We ran into a problem where we couldn't overwrite computed properties in tests. Some of our computed properties are very complicated and it would be easier to just overwrite them in tests rather than overwrite the depending properties.

This fixes #1888 and #1867)

PR FOR MOBX 5

@coveralls
Copy link

coveralls commented Jun 18, 2019

Coverage Status

Coverage increased (+0.009%) to 93.955% when pulling 2fd4540 on jillesme:mobx4-master-computed-configurable into a9061f4 on mobxjs:mobx4-master.

Copy link
Contributor

@danielkcz danielkcz left a comment

Choose a reason for hiding this comment

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

Please add a warning to docs it shouldn't be abused for regular code, as a test tool it certainly could be useful. Although, I would say that a test that also checks if computed is working properly is more robust than some hacked value.

You should also update changelog, otherwise LGTM.

package.json Outdated Show resolved Hide resolved
@jillesme
Copy link
Collaborator Author

@FredyC thank you for the quick response. I've updated the package.json and will create a separate PR on the gh-pages branch to update the docs.

jillesme added a commit to jillesme/mobx that referenced this pull request Jun 18, 2019
@jillesme jillesme mentioned this pull request Jun 18, 2019
@danielkcz
Copy link
Contributor

danielkcz commented Jun 18, 2019

I wonder, you have linked #1888 which is clearly about MobX 5 and yet you are doing this change in V4 only? Can you please also make another PR with merge to master? Normally it should be the other way around.

Besides, your docs PR is updating V5 docs, so it should definitely exist in that version :)

@jillesme
Copy link
Collaborator Author

You're right @FredyC - updating all related PRs.

@jillesme
Copy link
Collaborator Author

Last 4 PRs are mine... @FredyC - Is there anything else that needs to be done on my side?

@danielkcz danielkcz merged commit c059e28 into mobxjs:mobx4-master Jun 19, 2019
jillesme added a commit that referenced this pull request Jul 3, 2019
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.

3 participants