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 data shim so jQuery's data functions also respect domData. #43

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

bmomberger-bitovi
Copy link
Contributor

Posting this for discussion of implementation (though the implementation should work for most cases). This PR adds support for jQuery prototype .data() and .removeData(), and for static $.data(), $.hasData(), and $.removeData(). In each case, the domData cache from can-util will be examined or updated when the corresponding jQuery data function is called.

The jQuery data store takes precedence for getting named properties. For grabbing the full data object, the results from jQuery data and domData are merged together.

$.data()/.data() called as a setter and $.removeData()/.removeData() operate on both caches.

Addresses #42

Copy link
Contributor

@justinbmeyer justinbmeyer left a comment

Choose a reason for hiding this comment

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

I think this is some great work. We need to change that .js, and I recommend not pulling it in ... at least at this point for people who are using legacy, this could just throw a wrench in?

can-jquery.js Outdated
@@ -11,6 +11,7 @@ var makeArray = require("can-util/js/make-array/make-array");
var mutate = require("can-util/dom/mutate/mutate");
var setImmediate = require("can-util/js/set-immediate/set-immediate");
var canViewModel = require("can-view-model");
require("./data.js");
Copy link
Contributor

Choose a reason for hiding this comment

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

.js should probably be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should not include this right now, and document this module so people can add it. That way people can experiment with it? This might be a good way to start.

Copy link
Contributor Author

@bmomberger-bitovi bmomberger-bitovi left a comment

Choose a reason for hiding this comment

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

Good suggestions @justinbmeyer . I've implemented them. Please review and approve.

@bmomberger-bitovi
Copy link
Contributor Author

I had to fix how events were dispatched to make the tests pass. new KeyboardEvent() as used in a can-util test doesn't make the type property enumerable, which breaks the type identifier in jQuery's event dispatch process. now there's an explicit property check for type, when the event to dispatch is an object.

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.

2 participants