Improve can.scope and $.fn.scope code and add assignment support #740

Merged
merged 1 commit into from Feb 19, 2015

Conversation

Projects
None yet
3 participants
@zkat
Contributor

zkat commented Feb 16, 2014

This patch makes it so all of the following are supported:

$(el).scope() // => can.Map instance
$(el).scope("foo") // => fetches "foo" property of can.Map instance
$(el).scope("foo", value) // => sets value as the value for the "foo" property on that instance

as well as the can.scope() equivalents.

@zkat

This comment has been minimized.

Show comment
Hide comment
@zkat

zkat Feb 25, 2014

Contributor

Bump. I'm not sure why phantomjs died on this. Fluke? Is there a way to just rerun the tests, or do I need to force a push?

Contributor

zkat commented Feb 25, 2014

Bump. I'm not sure why phantomjs died on this. Fluke? Is there a way to just rerun the tests, or do I need to force a push?

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Feb 25, 2014

Contributor

It's been kind of flaky lately. Btw, is this a 2.0.6 or 2.1 feature?

Contributor

daffl commented Feb 25, 2014

It's been kind of flaky lately. Btw, is this a 2.0.6 or 2.1 feature?

@zkat

This comment has been minimized.

Show comment
Hide comment
@zkat

zkat Feb 25, 2014

Contributor

It extends the API as coded but is backwards-compatible. I would argue this behavior should have already been expected, which would make it a bug and thus a 2.0.6 fix, but it's really a backwards-compatible API extension, which would land it in 2.1.

I don't see any actual documentation for can.scope, and $().scope() is only casually mentioned because it happens to be used by http://canjs.com/docs/can.Component.html, so it was never technically a defined part of our API.

Contributor

zkat commented Feb 25, 2014

It extends the API as coded but is backwards-compatible. I would argue this behavior should have already been expected, which would make it a bug and thus a 2.0.6 fix, but it's really a backwards-compatible API extension, which would land it in 2.1.

I don't see any actual documentation for can.scope, and $().scope() is only casually mentioned because it happens to be used by http://canjs.com/docs/can.Component.html, so it was never technically a defined part of our API.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Feb 25, 2014

Contributor

Playing devil's advocate. I'm not sure we should add this. The scope object that is being get / set is a can.Map. This seems to hide that fact by suggesting a different interface than .attr.

One possibility is to overwrite jQuery's .attr on custom elements to allow getting and setting attributes like:

$("tab").attr("title","foo")

Or, we might want to only support .scope().attr(). It's long winded, but precise.

Also, for this to land, it should include docs.

Contributor

justinbmeyer commented Feb 25, 2014

Playing devil's advocate. I'm not sure we should add this. The scope object that is being get / set is a can.Map. This seems to hide that fact by suggesting a different interface than .attr.

One possibility is to overwrite jQuery's .attr on custom elements to allow getting and setting attributes like:

$("tab").attr("title","foo")

Or, we might want to only support .scope().attr(). It's long winded, but precise.

Also, for this to land, it should include docs.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Feb 25, 2014

Contributor

It would be certainly 2.1. It's not a bug fix. Not only is this exending APIs, it's extending APIs that haven't been previously documented.

Contributor

justinbmeyer commented Feb 25, 2014

It would be certainly 2.1. It's not a bug fix. Not only is this exending APIs, it's extending APIs that haven't been previously documented.

@zkat

This comment has been minimized.

Show comment
Hide comment
@zkat

zkat Feb 26, 2014

Contributor

I added it to my own project because I made the mistake of trying to do el.scope("foo", 1) several times. When something bites me enough times, I figure it's time to fix it, but obviously I am not all users, and it's fine if this isn't welcome. I probably won't bother documenting it if it's not, and we can just close the issue, though I like the way the interface works in my own code and will probably keep that.

Contributor

zkat commented Feb 26, 2014

I added it to my own project because I made the mistake of trying to do el.scope("foo", 1) several times. When something bites me enough times, I figure it's time to fix it, but obviously I am not all users, and it's fine if this isn't welcome. I probably won't bother documenting it if it's not, and we can just close the issue, though I like the way the interface works in my own code and will probably keep that.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Feb 26, 2014

Contributor

I'm not opposed to it. I'm playing devil's advocate. What about the alternative of overwriting jQuery's attr?

Contributor

justinbmeyer commented Feb 26, 2014

I'm not opposed to it. I'm playing devil's advocate. What about the alternative of overwriting jQuery's attr?

@zkat

This comment has been minimized.

Show comment
Hide comment
@zkat

zkat Feb 26, 2014

Contributor

Overwriting jquery's .attr() has much more serious implications with how can.Component work with attributes as an interface, so I'd rather not touch that because there's too many ways it could go -- if you're going to override .attr(), you would also probably want to get rid of $#scope() altogether, as well, since it would be redundant -- your element would become the actual data interface.

One example of a serious implication is how we interpret non-strings being assigned to these values, which I think has several legitimate answers, specially because of how these could interact with can.view templates and how they pass stuff in. I also think there's value in having an internal-data-storage interface that isn't affected by an HTML element's attributes. Having that would be a generalization of the currently-hardcoded id/class/etc exception hack we do for component data binding.

Long story short, I'd rather have something like this in the short term than have this be blocked by a different part of the API design that can be tackled separately from this particular one. Yes, $#scope already works like can.Map#attr, but overriding something that is designed for DOM manipulation is a much bigger change, and I wouldn't consider it to be in the scope of the relatively small, quality-of-life improvement this patch is trying to do, and I don't consider it worth blocking because of that, unless you intend to completely remove this function until we do The Right Thing™, whatever that ends up being, about attributes.

Contributor

zkat commented Feb 26, 2014

Overwriting jquery's .attr() has much more serious implications with how can.Component work with attributes as an interface, so I'd rather not touch that because there's too many ways it could go -- if you're going to override .attr(), you would also probably want to get rid of $#scope() altogether, as well, since it would be redundant -- your element would become the actual data interface.

One example of a serious implication is how we interpret non-strings being assigned to these values, which I think has several legitimate answers, specially because of how these could interact with can.view templates and how they pass stuff in. I also think there's value in having an internal-data-storage interface that isn't affected by an HTML element's attributes. Having that would be a generalization of the currently-hardcoded id/class/etc exception hack we do for component data binding.

Long story short, I'd rather have something like this in the short term than have this be blocked by a different part of the API design that can be tackled separately from this particular one. Yes, $#scope already works like can.Map#attr, but overriding something that is designed for DOM manipulation is a much bigger change, and I wouldn't consider it to be in the scope of the relatively small, quality-of-life improvement this patch is trying to do, and I don't consider it worth blocking because of that, unless you intend to completely remove this function until we do The Right Thing™, whatever that ends up being, about attributes.

@zkat

This comment has been minimized.

Show comment
Hide comment
@zkat

zkat Jun 17, 2014

Contributor

Are we interested in this, or should we close it?

Contributor

zkat commented Jun 17, 2014

Are we interested in this, or should we close it?

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jun 18, 2014

Contributor

I'm interested. I'd like to include docs for it before merging into minor.

Contributor

justinbmeyer commented Jun 18, 2014

I'm interested. I'd like to include docs for it before merging into minor.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jun 18, 2014

Contributor

I'm being sneak w/ the docs request b/c I'm not sure can.scope is documented at all ...

Contributor

justinbmeyer commented Jun 18, 2014

I'm being sneak w/ the docs request b/c I'm not sure can.scope is documented at all ...

@zkat

This comment has been minimized.

Show comment
Hide comment
@zkat

zkat Jun 19, 2014

Contributor

Added docs and rebased onto the latest master

Contributor

zkat commented Jun 19, 2014

Added docs and rebased onto the latest master

@zkat

This comment has been minimized.

Show comment
Hide comment
@zkat

zkat Feb 16, 2015

Contributor

@daffl all done

Contributor

zkat commented Feb 16, 2015

@daffl all done

@daffl daffl added this to the 2.2.0 milestone Feb 19, 2015

component/component.js
+
+ // If there is a `$` object and it has the `fn` object, create the
+ // `scope` plugin that returns the scope object.
+ if (window.$ && $.fn) {

This comment has been minimized.

@daffl

daffl Feb 19, 2015

Contributor

I think this needs to be window.jQuery

@daffl

daffl Feb 19, 2015

Contributor

I think this needs to be window.jQuery

daffl added a commit that referenced this pull request Feb 19, 2015

Merge pull request #740 from bitovi/scope-fn-assignment
Improve can.scope and $.fn.scope code and add assignment support

@daffl daffl merged commit 6edbf7a into master Feb 19, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@daffl daffl deleted the scope-fn-assignment branch Feb 19, 2015

@daffl daffl removed the fixed in branch label Feb 20, 2015

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