Skip to content

ability to specify a type for derived properties - #116

Open
flipside wants to merge 4 commits into
AmpersandJS:masterfrom
flipside:derived-type
Open

ability to specify a type for derived properties#116
flipside wants to merge 4 commits into
AmpersandJS:masterfrom
flipside:derived-type

Conversation

@flipside

@flipside flipside commented Dec 6, 2014

Copy link
Copy Markdown

By specifying a dataType for derived properties, a derived property with type:"state" can dynamically have listeners updated when it returns a different state.

Derived types also allow for custom behavior by adding new dataTypes.

@bear bear added the request label Dec 7, 2014
@latentflip

Copy link
Copy Markdown
Contributor

Hey @flipside, thanks for submitting this! I think I'm heading towards +1 on this idea. I've a couple of thoughts though - forgive me if I'm being slow.

First up: what does init: true do here? Do we need it?

@aaronmccall

Copy link
Copy Markdown

@latentflip, by my read it looks like init: true is intended to ensure that the derived prop is populated to the cache at instantiation rather than being lazy.

@flipside

flipside commented Dec 8, 2014

Copy link
Copy Markdown
Author

@aaronmccall @latentflip i wasn't sure if it should always automatically set a derived property on load if a type is set so I made them separate params. i'm open for init to default to true when a type is specified but i can imagine situations where it would be important to override.

@flipside

flipside commented Dec 8, 2014

Copy link
Copy Markdown
Author

the other thing worth discussing is that since this approaches uses dataType, should it go all the way with using get/set/default and not just compare?

@flipside flipside mentioned this pull request Dec 9, 2014
@flipside

Copy link
Copy Markdown
Author

adding support for default and the rest of the dataType functions turned out to be easy and didn't break tests so i added them.

Comment thread ampersand-state.js

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason we're getting rid of flushCache?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm guessing cos it's not used. Doesn't appear to be being used by any other modules as per a github search. What do you think about removing it @HenrikJoreteg? Can you remember if it's useful/needed?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know at one point or another i wanted to be able to do this forcibly. But can't remember why. If it's not being used and everything passes, then we're probably good.

@flipside

Copy link
Copy Markdown
Author

one debatable part is supporting default. removing it from this update would simplify things a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants