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

decorate - compose decorators for a single prop #1652

Merged
merged 4 commits into from
Aug 17, 2018

Conversation

ramybenaroya
Copy link
Contributor

PR checklist:

  • Added unit tests
  • Updated changelog
  • Updated docs (either in the description of this PR as markdown, or as separate PR on the gh-pages branch. Please refer to this PR). For new functionality, at least API.md should be updated
  • Added typescript typings
  • Verified that there is no significant performance drop (npm run perf)

Changes

  • Reduce multiple decorators to a single decorator
  • Added a test for multiple decorators (@action + custom) on a function property
  • Added a test for multiple decorators (@observable + @serializable) on
    a regular property
  • Added a usage example (+ caveat warning) in readme under decorate section

Usage Example:

import { decorate, observable } from "mobx"
import { serializable, primitive } from "serializr"
import persist from 'mobx-persist';

class Todo {
    id = Math.random();
    title = "";
    finished = false;
}
decorate(Todo, {
    title: [serializable(primitive), persist('object'), observable],
    finished: observable
})

- Reduce multiple decorators to a single decorator
- Added a test for multiple decorators (@action + custom) on a function property
- Added a test for multiple decorators (@observable + @serializable) on
  a regular property
- Added a usage example in readme under `decorate` section
@@ -67,6 +67,7 @@
"rollup": "^0.41.6",
"rollup-plugin-filesize": "^1.3.2",
"rollup-plugin-node-resolve": "^3.0.0",
"serializr": "^1.3.0",
Copy link
Contributor Author

@ramybenaroya ramybenaroya Jul 25, 2018

Choose a reason for hiding this comment

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

I added serializr as a dev dependency just for this test, so I can compose @observable with another decorator @serializable(primitive()).

Copy link
Member

Choose a reason for hiding this comment

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

N.P.

README.md Outdated
finished: [serializable(primitive), observable]
})
```
Note: Composing decorators can sometimes lead to strange things when one decorator modifies the [property descriptor](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty) in a way that is unexpected for the other decorators.
Copy link
Contributor Author

@ramybenaroya ramybenaroya Jul 25, 2018

Choose a reason for hiding this comment

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

Not so sure if my explanation is clear enough. Would love to get a feedback about it.
After approval I'll open a gh-pages PR

Copy link
Member

Choose a reason for hiding this comment

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

Note: not all decorators can be composed together, and this functionality is just best-effort. Some decorators affect the instance directly and can 'hide' the effect of other decorators that only change the prototype

@coveralls
Copy link

coveralls commented Jul 25, 2018

Coverage Status

Coverage increased (+0.02%) to 93.92% when pulling 2e07c95 on ramybenaroya:compose-property-decorators into f76f198 on mobxjs:master.

README.md Outdated
@@ -107,6 +107,24 @@ decorate(Todo, {
})
```

For applying multiple decorators on a single property, you can pass an array of decorators.
Copy link
Member

Choose a reason for hiding this comment

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

Please mension what the application order is. I assume serializable is applied first, then persist and finally observable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct

@mweststrate
Copy link
Member

Some small comments, further looking great!
Note that if you need to support this feature for MobX, a second PR targeting mobx4-master should be opened.

Thanks!

@ramybenaroya
Copy link
Contributor Author

Thanks for the feedback @mweststrate.

if you need to support this feature for MobX...

I didn't get what you meant by that. Can you elaborate about "support for MobX"?

@mweststrate
Copy link
Member

@ramybenaroya sorry, meant MobX 4. In other words; master represents 5, but 4 is still maintained as well so it might be interesting to backport this one. (if this branch is merged with squash, it can probably just be cherry-picked)

- Change caveat note about docorators composition as suggested in
  mobxjs#1652 (comment)
- Add decorators application order
ramybenaroya added a commit to ramybenaroya/mobx that referenced this pull request Aug 15, 2018
This is documentation for changes in
mobxjs#1652
@mweststrate mweststrate merged commit 433452a into mobxjs:master Aug 17, 2018
mweststrate pushed a commit that referenced this pull request Aug 17, 2018
* docs: Add example for composing decorators

This is documentation for changes in
#1652

* Update api.md

* use single quotes

* Update api.md
ramybenaroya pushed a commit to ramybenaroya/mobx that referenced this pull request Aug 19, 2018
* decorate - compose decorators for a single prop

- Reduce multiple decorators to a single decorator
- Added a test for multiple decorators (@action + custom) on a function property
- Added a test for multiple decorators (@observable + @serializable) on
  a regular property
- Added a usage example in readme under `decorate` section

* delete package-lock

* doc: update README

- Change caveat note about docorators composition as suggested in
  mobxjs#1652 (comment)
- Add decorators application order

* Update README.md
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