Skip to content

Allow properties on functions to be watchable/streamable#11642

Closed
machty wants to merge 1 commit intoemberjs:masterfrom
machty:watchable-functions
Closed

Allow properties on functions to be watchable/streamable#11642
machty wants to merge 1 commit intoemberjs:masterfrom
machty:watchable-functions

Conversation

@machty
Copy link
Contributor

@machty machty commented Jul 2, 2015

Allows things like {{foo.bar}} to bind properly when foo is a function

Use cases: the function form of
emberjs/rfcs#2

Copy link
Member

Choose a reason for hiding this comment

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

Maybe export this, so you can use it below in key-stream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did until async dependency undefineds made me want to cry to the point of sneaking this in. I can take a stab at it again when I emotionally recover.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, understood, makes sense. This is fine then...

@rwjblue
Copy link
Member

rwjblue commented Jul 3, 2015

👍 - just left a few small comments

@stefanpenner
Copy link
Member

likely needs to be featureflagged unless it is deemed a bugfix.

@stefanpenner
Copy link
Member

cc @krisselden

@rwjblue
Copy link
Member

rwjblue commented Jul 3, 2015

@stefanpenner - It seems like a bugfix to me...

@mixonic
Copy link
Member

mixonic commented Jul 3, 2015

seems good, @machty travis is complaining.

@stefanpenner
Copy link
Member

@rwjblue ya, sounds good.

@machty
Copy link
Contributor Author

machty commented Jul 3, 2015

Will fix the issues shortlay.

@machty
Copy link
Contributor Author

machty commented Jul 3, 2015

Not sure if this is really a big deal but it surprised me that my first stab at a test case failed due to expecting the name property of a function to behave like any ol property on an object, when it is actually read only. Just throwing that out there in case it jogs someone else's memory as to other issues with this PR.

@stefanpenner
Copy link
Member

name, length, caller are non-observable/writable function properties.

Can I understand the use-case where observing a function is actually desired?
Although it sorta appears reasonable, I can only think of anti-patterns use-cases. If you want state, make and object?

@machty
Copy link
Contributor Author

machty commented Jul 4, 2015

@stefanpenner my old better actions rfc outlines use cases.

Basically I want to be able to support the pattern of

<form {{action submitForm on="submit"}}>
  {{#if submitForm.pending}}
    Please wait...
  {{else}}
    {{! bunch of input fields}}
  {{/if}}
</form>

where submitForm is a function that encapsulate a singleton action, the async state of which multiple parties care about. I've been using it to great effect in my own app, it cuts down on a ton of error prone boilerplate, and it reads nicer than tons of lispy (is-pending) sexprs (not to mention you want to be able to bind the state of the promise/sequence in JS, which means a separate isPending API). In my app, submitForm and the like are actually objects with a .perform(), but as @mixonic's Improved Actions RFC takes hold, it'd be nice to maintain API parity of passing in an Action type (whether it's an Improved Action or one of my old school Better Actions) to component and just be able to call it as this.attrs.someAction() while, in the Better Actions, case, being able to still bind to and get the value of this.attrs.isPending.

@stefanpenner
Copy link
Member

@machty ya still feels kinda funky, but you are likely correct.

@mixonic
Copy link
Member

mixonic commented Jul 6, 2015

I'm not sold on @machty's inclination for putting the pending property on the action function, but regardless I am +1 for this patch! It seems reasonable.

@machty
Copy link
Contributor Author

machty commented Jul 6, 2015

I might not even be sold on it but can't experiment without it and seems like it should work either way.

@stefanpenner
Copy link
Member

It seems reasonable.

ya, although quirky, i think it totally reduces WATs without introducing much cost at all

Sorry @machty if you felt i was being overly negative. My goal was to fill in my own blanks.

@stefanpenner
Copy link
Member

@machty ping

Allows things like {{foo.bar}} to bind properly
when foo is a function

Use cases: the function form of
emberjs/rfcs#2
@machty machty force-pushed the watchable-functions branch from 4070c7e to adbb301 Compare July 14, 2015 16:53
@machty
Copy link
Contributor Author

machty commented Jul 15, 2015

Sorry, finally had some time to dig into the failing tests a bit more... there are some CollectionView tests that are failing because there is internal logic that expands to checking/watching view._emptyView.length, and _emptyView is one of these ugly legacy things that can either be a factory or an instance, and when it's a factory (a function), the code I added starts making it watch Function.length and eventually gets browser/platform-specific errors about non-configurable properties and defineProperty.

I'm kind of ready to throw in the towel on this one unless folk actually think this is a useful thing to have around. I don't think it's really worth much deeper investigating, but if someone more familiar with some of these internals has a moment to pair, maybe it's an easy thing to fix.

@stefanpenner
Copy link
Member

I'm unsure how to fix this. Without introducing special casing for function specific built ins.

The scenario you describe may actually be worth cleaning up and removing from ember. Although that may be more work then you are interested in.

Silly edge cases

@machty machty closed this Jul 15, 2015
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.

4 participants