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

console.warn for .hide() and other perf gotchas. #2250

Closed
jdalton opened this issue Apr 30, 2015 · 26 comments
Closed

console.warn for .hide() and other perf gotchas. #2250

jdalton opened this issue Apr 30, 2015 · 26 comments

Comments

@jdalton
Copy link
Member

jdalton commented Apr 30, 2015

There's a known performance issue with hide & friends that jQuery would like to fix.

I've seen browsers leverage console warning used to create positive change.
For example Firefox warns of __proto__ use and this got Zepto to drop it.

I think this same technique could be used to deliver a gentle one-off warning about hide's use, or the underlying issue, and recommend an upgrade path.

I'd like to flesh out this idea for browser dev tools here for easier collaboration.

\cc @paulirish

@dmethvin
Copy link
Member

Based on previous experience with console warnings, people get really crazy if they get a warning and there is no resolution to fix it. So if we can coordinate our work here to make it so that the resolution was to upgrade (for example to jQuery 3.x) it would make life better for everyone! I'm not sure how practical that would be but it would be really interesting to explore.

@scottgonzalez
Copy link
Member

I'm strongly against this. There are valid uses and it should not warn.

@jasonkarns
Copy link

I could see this being beneficial in a development build or behind a flag, but would be very upset if jquery were logging from a production build.

@geedew
Copy link

geedew commented Apr 30, 2015

Deprecation warnings would be okay; if that were feasible. I would request anything like this should be present in an alternate build. Maybe a more chatty 'dev' build could be done for active development notes.

@dmethvin
Copy link
Member

This isn't a proposal for jQuery to log, it's the browser detecting that you are doing bad things from a perf perspective and warning about it. The one @jdalton gives regarding Zepto is a good one.

@paulirish
Copy link
Member

I think we should explore this, but don't want to just spam the console on any method that I or someone else called "slow".

side note: I just did a talk about how all "slow"s are not equal and we need to prioritize what the user feels

There are two hard parts to this: 1) Detecting the troublesome uses. 2) Giving the user realistic guidance on what action to take.

We have to be thoughtful though, as it's a very serious thing to drop warnings into the console.

(And yes, this is basically a feature request for Chrome/FF/Microsoft Edge/Safari devtools teams, rather than something that jquery.js implements)

@bloodyowl
Copy link

👍

react has the same kind of very expressive warnings & errors, which is really good for both the learning curve, and the upgrades (I love it and lots of people I've spoken to about this love it too). I think this is a good habit, and can be really useful for deprecation & performance warnings for educating users (& of course this should only occur with the production mode).

IMO jQuery has completely failed to educate developers so far, because of a non-explicit API & missing the occasions of reducing the API to get close to the DOM when possible. now would be the time to start be more than an insurance for browser compat.

@dmethvin
Copy link
Member

IMO jQuery has completely failed to educate developers so far,

Not sure what this means. The code isn't about education, it's providing functionality via the API. The docs provide more guidance about how and when to use the APIs. The jQuery Migrate plugin provides a lot more information about practices and APIs that aren't recommended in new code.

I agree with @paulirish that this can't be a "this API is bad and you should feel bad" message. It needs to be more about providing guidance for fixing problems that can be detected while a page runs.

@jdalton
Copy link
Member Author

jdalton commented Apr 30, 2015

I agree with @paulirish that this can't be a "this API is bad and you should feel bad" message. It needs to be more about providing guidance for fixing problems that can be detected while a page runs.

Yap, agreed too!

@bloodyowl
Copy link

what I mean is that:

by having "polymorphic" methods like jquery.css instead of jquery.getStyle & jquery.setStyle, it makes it more difficult to teach someone who started front-end development with jQuery to name & code explicitly.

by having a completely fucked-up this value in forEach, map & filter, same people who are introduced to JavaScript by jQuery got easily confused about this conventions in ecma standard methods & jQuery.

by having naming not evolving with convention (like $.proxy vs Function.prototype.bind) it doesn't help people to catch-up with the spec, and instead adds confusion.

and by not trying to align to the DOM spec (about naming & returned values signature) as close as possible, not educating people to learn how to do it without jQuery might be dangerous. if you have the hope that ideally, jQuery will no longer be necessary in the future, offering people some DOM knowledge becomes a necessity.

@stucox
Copy link

stucox commented Apr 30, 2015

If you don't want people to use it, deprecate & remove it.

@miketaylr
Copy link

This kind of info might also be interesting in devtools performance timeline tooling.
(cc @jsantell @victorporof @canuckistani)

@jdalton
Copy link
Member Author

jdalton commented Apr 30, 2015

This kind of info might also be interesting in devtools performance timeline tooling.

Cool, thanks for pinging in! May be more suitable there depending on how complicated detection is.

@paulirish mentioned Chrome may be able to measure the time of the jank and walk the stack too see if it's because of jQuery hidden checking.

@dmethvin
Copy link
Member

@stucox in general I agree, and we've deprecated/removed several APIs over the years. The problem comes with complex APIs like $.ajax() or $.fn.show() where behaviors for edge cases cost us a lot in performance, bytes, and/or confusing documentation. We want to keep the baby but throw out the bathwater.

@miketaylr I definitely think this is where devtools can add value. We don't want to spew on the console for opinionated purity-of-code reasons but we want to give people better guidance when they're doing something that is causing bad performance.

@neemzy
Copy link

neemzy commented May 2, 2015

@bloodyowl Amen.

Yes, when your library is jQuery, you do have responsibility towards the community ; pretending you don't is straight blind ignorance.

As for the warning, yes, way to go. It helps a lot with React, given it has the advantage of knowing your environment (dev or prod) based on a node.js env variable. You won't be able to do the same, but let's be honest, who "builds" jQuery from source ? Just offer different builds to download and be very explicit about this (I suggest one env variable to rule them all, for similar future cases).

@timmywil
Copy link
Member

timmywil commented May 4, 2015

I think the direction is good here, but it doesn't seem like there's anything to be done within jQuery core. The action items are directed at browser development tools. Is that accurate @jdalton?

@jdalton
Copy link
Member Author

jdalton commented May 4, 2015

Correct. I've opened it here for easier cross browser vendor collaboration and to get input from jQuery core. If warning were implemented browsers would want input on what to direct users to (a newer version of jQuery perhaps).

@timmywil
Copy link
Member

timmywil commented May 4, 2015

By the way, I should mention here that, in the case of .show/.hide, we have sweeping changes prepared for 3.0.

@jdalton
Copy link
Member Author

jdalton commented May 4, 2015

By the way, I should mention here that, in the case of .show/.hide, we have sweeping changes prepared for 3.0

Yap, referenced in the opening comment. One of the things mentioned was coordinating the warning with a release to be able to point devs to solution/workaround for the issue.

@jsantell
Copy link

jsantell commented May 4, 2015

Thanks for the heads up, @miketaylr -- pinged a few other Firefox DevToolers to this thread and we'll be looking into any warnings that can be provided when there are faster, optimized alternatives (like the proto case in the OP)

@markelog
Copy link
Member

markelog commented May 4, 2015

Correct. I've opened it here for easier cross browser vendor collaboration and to get input from jQuery core

Well if you do, then there is mine -

personally, i resent the statement of jQuery#(show | hide) being slow, it simple isn't, DOM API is. Those methods consider great amount of edge-cases to make the end-user happy, now we will cut them out and thus drastically break a lot of use-cases - this is now a necessary sacrifice to make it faster.

Users though, oh, users will face great issues while updating to the new jQuery since their apps/sites will break in unexpected places, yeah, i wonder who they will blame for it?

But punch line is - we didn't have to do that, if DOM API would have a "right" API, jQuery methods could have been fast and could have been thorough.

I will you give couple examples:

  1. Thanks to the @bzbarsky firefox introduced the window.getDefaultComputedStyle which given us to avoid this crazy slow logic that is now still used in jQuery(show | hide), but no one standardize this method, no one beside the ff implemented it and honestly, it didn't work right, since we still had to do some hacks to make it work.
  2. insertAdjacentHTML, great method right? Simplifies and speed up a lot of use-cases? Perfectly aligned with jQuery manipulation API, right? No, no it is not, too tricky, lots of edge-cases that were not considered, can't be used.
  3. Element Traversal API, in some ways mimics the jQuery traversal API, gotta to be the right API to use inside jQuery? Nope, specification isn't thorough, doesn't consider non-element nodes and now this API is obsolete.
  4. requestAnimationFrame awesome method to make things faster, but why we had to wait four years before reintroducing it? Because without of conjunction with Page Visibility API this method just doesn't bring the needed effect. One edge-case and everything breaks. But while we waited, we received all kinds of storms criticism for "slow" jQuery animation API.

I could add a lot more examples like that, like am not gonna be surprised if Element#closest tryout will have the same fate as examples above, while soon there is great possibility of jQuery#offset to be claimed as "slow", i can already imagine nice tweets flowing around blaming us for it.

Yeah, you could say jQuery Core members could have been more active in W3C discussions, could have advocate their position and we do, but most of us do this as a side thing, most of us have real jobs, whereas those spec threads are time consuming, unrewarding and quite possibly - without expected end-result.

I don't think any warns are necessary, since at least currently, (@timmywil pull you mentioned, already in the this ticket description btw) jQuery#(show | hide) do really weird things while using various APIs to achieve desirable effect, having problems of imagining the code-path you would need to take to predict it "slowness".

I do think right DOM API could have been introduced that would relieve us from very circumstance of warning people about anything.

@timmywil
Copy link
Member

timmywil commented May 4, 2015

One of the things mentioned was coordinating the warning with a release to be able to point devs to solution/workaround for the issue.

Right, I'm just saying that, barring having to revert for reasons unknown at this time, 3.0 should be the right release to point to.

Edit: it's my understanding that Paul Irish's analysis and, subsequently, my own, revealed that the bottleneck in that example was retrieving the computed display value. Webkit took the most time in getPropertyStyle. With the proposed changes for 3.0, we drop that and use element.style instead, effectively circumventing the performance issue. The other issue that I was concerned about had to do with the css cascade and overriding stylesheet display: none settings. This required a lot of magic to get working – and why I named defaultDisplay as the likely performance culprit originally. We will drop that too in 3.0. .show/.hide will become very basic methods that should no longer have the same potential performance penalties (at least not ones that we have encountered so far). This does mean breaking changes, but we'd rather break some code than continue to allow these phantom performance issues to sneak into user code.

@paulirish
Copy link
Member

I now have a ticket filed on Chromium for this request: https://code.google.com/p/chromium/issues/detail?id=503853#c4 It's profiling focused, so that the recommendations/warnings are only provided if a problem is detected.

@neemzy
Copy link

neemzy commented Jun 25, 2015

But punch line is - we didn't have to do that, if DOM API would have a "right" API, jQuery methods could have been fast and could have been thorough.

If that was the case, we would not have needed jQuery in the first place, would we? At least for such simple things.

@jdalton
Copy link
Member Author

jdalton commented Jun 25, 2015

@paulirish

I now have a ticket filed on Chromium for this request

Awesome, thank you!

@timmywil
Copy link
Member

timmywil commented Dec 9, 2015

Since this discussion hasn't continued here, I'll close this ticket. We have different plans for show/hide in 3.0 that should still address the performance issue for general cases, but will not address it for all cases. We will not be adding console warnings in jQuery for this, but this ticket wasn't really suggesting that. As for browser vendors, I don't think there should be a warning every time someone uses hide or show, but we seem to have mostly agreed on that part. Browser vendors can/should provide recommendations, but only when there is actually a problem. It may be hard to detect sometimes, but no matter the difficulty in detecting an actual problem, that is their task, and it is a noble one.

@timmywil timmywil closed this as completed Dec 9, 2015
@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