Skip to content
This repository has been archived by the owner on Feb 17, 2022. It is now read-only.

Add a possibility to work with setter of mobx fields #14

Merged
merged 6 commits into from
Nov 14, 2017
Merged

Add a possibility to work with setter of mobx fields #14

merged 6 commits into from
Nov 14, 2017

Conversation

lacedon
Copy link

@lacedon lacedon commented Nov 8, 2017

#13

I add a possibility to add setter like this:

{
  ...
  fromMobx: {
    localFieldName: {
      get() { return storeInstance.storeFieldName },
      set(value) { storeInstance.setStoreFieldName(value) }
    }
  },
  ...
}

@nighca
Copy link
Owner

nighca commented Nov 13, 2017

Thanks for your work @lacedon .

Seems that you used vue's computed property to realize setter for fromMobx fields. To achieve this, another reactive property (__movue_${fieldKey}) is introduced. That makes it a little too complicated to understand and debug.

I have be wondering if we can realize setter without introducing vue computed property, but failed until now. What about introducing computed only when setter is configured?

Since we rarely need setter (maybe I'm wrong, but I do not like the getter/setter way), we can get clean realization in most times.

Hope for your idea.

src/install.ts Outdated
})

vm[movueDisposersKey] = disposers
Copy link
Owner

@nighca nighca Nov 13, 2017

Choose a reason for hiding this comment

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

Another thing I care about here: when there is no fromMobx part in the component definition, it will be better for us not to set the __movueDisposers__ field of the component. That makes normal components cleaner.

Copy link
Author

Choose a reason for hiding this comment

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

I agree. I will refactor that code.

@lacedon
Copy link
Author

lacedon commented Nov 13, 2017

Seems that you used vue's computed property to realize setter for fromMobx fields. To achieve this, another reactive property (__movue_${fieldKey}) is introduced. That makes it a little too complicated to understand and debug.

I have be wondering if we can realize setter without introducing vue computed property, but failed until now. What about introducing computed only when setter is configured?

I agree, that it's little bit complicated, but I'm not sure about debugging.
As for end user, computed property could be more useful, because they can see computed properties in vue dev tool. But vue dev tool doesn't show "just reactive" properties.

Also I think if we do that in one style it will be easier to understand code of plugin.

And beside that, if component have data property with name that's the same as reactive property there won't be any message about that. But if we use computed property Vue will throw warning about the same names. I think, logic like that should be part of Vue, not plugins.

@nighca
Copy link
Owner

nighca commented Nov 13, 2017

I agree that computed is better for users.

There is another way that extra __movue_${fieldKey} properties can be avoided: create a new Vue instance and define all reactive properties on it (just like what vuex does), then we can use them in the computed fields of given components.

@lacedon
Copy link
Author

lacedon commented Nov 13, 2017

Great idea!

I created a file change-detector.ts and put in it code to work with reactive properties.

src/install.ts Outdated
}

changeDetector.removeReactionList(vm)
getFromStoreEntries(vm).map(({ key }) => changeDetector.removeReactiveProperty(vm, key))
Copy link
Owner

@nighca nighca Nov 14, 2017

Choose a reason for hiding this comment

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

Maybe forEach will be better here than map

Copy link
Author

Choose a reason for hiding this comment

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

I agree. Fixed that.

Copy link
Owner

@nighca nighca left a comment

Choose a reason for hiding this comment

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

👍

@nighca nighca merged commit cf2f331 into nighca:master Nov 14, 2017
@nighca
Copy link
Owner

nighca commented Nov 14, 2017

v0.1.0

@nighca nighca mentioned this pull request Nov 29, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants