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

Add pre-hook for properties #3234

Closed
wants to merge 3 commits into from

Conversation

TimvdLippe
Copy link
Contributor

Add pre-hooks for properties to validate and transform properties.
Adds a new field to a property called set that references the name of a function. This function takes a value and returns the new value.

Fixes #3099

@TimvdLippe TimvdLippe changed the title Add set-hook for properties Add pre-hook for properties Dec 24, 2015
@TimvdLippe
Copy link
Contributor Author

Fixed a stack-overflow issue when a child notified an binding update.

Effects of property update could re-process the setter.
To prevent this, only 1 call is allowed to be processed at the same time.
@BLamy
Copy link

BLamy commented Dec 28, 2015

👍 This is a lot cleaner then my original proposal.

// When setting the value, effects are triggered. In order to
// prevent stackoverflows, do not re-process the updated value
if (!processing) {
processing = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you actually need this lock/flag only b/c your setFn's in the tests are actually not idempotent?

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be wrapped by a try () finally ( ...releaseLock... )?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The stack-overflow error can be triggered quite easily. If the parent has defined a property with a setter and binds this to a property of a child, when the child property is updated the stack-overflow error is created.

Actually I was thinking of leaving this change out (so only pre-hooks, no transformation magic) as the cons might outweigh the pros.

@kevinpschaaf kevinpschaaf added the p0 label Feb 4, 2016
@kevinpschaaf kevinpschaaf self-assigned this Feb 4, 2016
@sorvell
Copy link
Contributor

sorvell commented Feb 4, 2016

This PR may fit this category: #3369.

@kevinpschaaf
Copy link
Member

With a feature like this, there will be a high temptation for users to depend on other element properties (e.g. this.min, this.max, etc.) in their validation logic, but this provides no way to declare those dependencies to ensure that a.) the values have been configured or b.) the validation is re-run when the dependencies change.

As-is, I don't think this would solve enough use cases to warrant an addition to the API surface right now. Computed properties solve much of this problem space, both the dependency issue and the efficiency issue related to taking action on the unvalidated value, by differing the value on the API from the one the element actually uses internally. As has been well stated, this comes at the cost of extra property definitions which we understand can feel burdensome. It's a hard problem so we'll continue to consider ways to make this pattern easier.

@TimvdLippe TimvdLippe deleted the pre-hook-property branch February 5, 2016 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants