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

Do not expose jQuery.domManip method #2225

Closed
markelog opened this issue Apr 21, 2015 · 13 comments
Closed

Do not expose jQuery.domManip method #2225

markelog opened this issue Apr 21, 2015 · 13 comments
Assignees
Milestone

Comments

@markelog
Copy link
Member

Since we get this opportunity only in major version, i'm gonna label it as a blocker

@markelog markelog added this to the 3.0.0 milestone Apr 21, 2015
@timmywil timmywil self-assigned this May 5, 2015
@justinbmeyer
Copy link
Contributor

Sorry about the late comment, but this is important for CanJS.

https://github.com/bitovi/canjs/blob/master/util/jquery/jquery.js#L140

It's how we are able to add an "inserted" event everytime new DOM is added to the page.

Anyway, this could be made public again in future releases?

@dmethvin
Copy link
Member

@justinbmeyer can you open a ticket for this? I think we should look at the problem from a functionality/capability standpoint rather than just re-exposing this internal method.

@dmethvin
Copy link
Member

dmethvin commented Jan 7, 2016

@justinbmeyer is CanJS still using .domManip? Since it's an undocumented method we were planning to remove it in 1.12/2.2 but we can extend that to 3.0 if we hear from you in the next day.

@dmethvin dmethvin modified the milestones: 1.12/2.2, 3.0.0 Jan 7, 2016
@justinbmeyer
Copy link
Contributor

Yes, we are still using it. In about 6 months, we are finally dropping support for browsers that don't support MutationEvents/Observers. At that point, we won't need it exposed anymore. Thank you very much for keeping it around.

@dmethvin
Copy link
Member

dmethvin commented Jan 8, 2016

Reopening as a reminder

@dmethvin dmethvin reopened this Jan 8, 2016
@mgol
Copy link
Member

mgol commented Jan 8, 2016

The milestone here is a little misleading as we're now planning to drop it only in 3.0.0. :) But OK, it's a good reminder and we can bump the milestone back later.

@dmethvin
Copy link
Member

dmethvin commented Jan 8, 2016

All the "Do not expose" tickets may need to be re-exposed for 1.12/2.2 but I figured we should discuss. Rather than reverting the commits it would be easier to simply re-expose each via something like jQuery.domManip = domManip and a comment that the method is deprecated and will be removed in 3.0.

timmywil added a commit that referenced this issue Jan 8, 2016
timmywil added a commit that referenced this issue Jan 8, 2016
@timmywil
Copy link
Member

timmywil commented Jan 8, 2016

Fixed in 1.12/2.2

@timmywil timmywil closed this as completed Jan 8, 2016
@timmywil timmywil modified the milestones: 3.0.0, 1.12.0/2.2.0 Jan 8, 2016
@timmywil
Copy link
Member

timmywil commented Jan 8, 2016

Changed milestone back to 3.0 as they're still going to be privatized in 3.0.

@justinbmeyer
Copy link
Contributor

@dmethvin I should have noticed this earlier today. But I need to overwrite domManip so I can get a hook for anytime jQuery modifies the dom. Simply exposing the method doesn't work because changing jQuery.fn.domManip doesn't effect all of jQuery's calls to domManip.

Is there anyway to get this changed, so that $.fn.domManip is being called eveywhere?

(this hook is far more important than buildFragment, it supports our "inserted" and "removed" custom events).

@dmethvin
Copy link
Member

dmethvin commented Jan 8, 2016

I think this would need to be discussed with the team. Is this a temporary reprieve for 1.x/2.x, or do you need a feature to hook DOM changes for 3.0 as well? We're not planning on anything other than patch releases for 1.x/2.x from here on out and weren't expecting to expose any of these for 3.0.

@justinbmeyer
Copy link
Contributor

A temporary reprieve. CanJS 3.0 will start using MutationObservers. It will be able to support "inserted" and "removed" events without hijacking jQuery.

I could work around this by overwriting all of jQuery's modifier methods. However, I'd need $.buildFragment to be able to convert the string arguments to a fragment.

@justinbmeyer
Copy link
Contributor

@dmethvin I'm able to work around this. So no need to add it back.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

5 participants