search

jquery / contribute.jquery.org

Developer documentation common to jQuery projects
https://contribute.jquery.org
Other
25 stars 76 forks source link

Document style of "support" comments #95

Open markelog opened 9 years ago

markelog commented 9 years ago

Every time we have a rewrite or drop support for some browser we engage (at least in Core) in discussion of how to properly write those support comments:

Support: IE8<9 IE8+ IE11< ...

We need clear understanding of how to write those comments, so we can leave those discussion behind and focus on code instead of flaming the pull/issue thread with pointless posts.

mgol commented 9 years ago

:+1: to documenting that. This is my understanding of current de-facto rules (a little extended) that we could codify on a case by case basis (I'm omitting the // Support: prefix and assume that data for specific browsers is separated using ,):

  1. Browser has a bug in versions k-n, we don't know if it was fixed in n+1:
Browser k-n+
  1. Browser has a bug in versions k-n, it's fixed in n+1:
Browser k-n
  1. Browser has a bug in version n, we don't know if it was fixed in n+1:
Browser n+
  1. Browser has a bug in version n, it's fixed in n+1:
Browser<n+1
  1. Browser has a bug in version k, n (k < n but maybe also k < n-1), we don't know if it was fixed in n+1:
Browser k, n+
  1. Browser has a bug in version k, n (k < n but maybe also k < n-1), it was fixed in n+1:
Browser k, n

or

Browser<n+1

(the latter ignores information about version k)

This convention would mean that unless the last version mention is followed by +, we assume it was fixed in the next version.

The main drawback of this convention is IMO that Browser n and Browser<n+1 seems interchangeable and we should have a concrete convention everywhere.

This is important for every project so cc @jquery/core @scottgonzalez @arschmitz @jzaefferer.

mgol commented 9 years ago

BTW, if we don't come up to a concrete conclusion quickly I'd like to merge https://github.com/jquery/jquery/pull/1837 in its current form; it's quite large and I don't want to have to continue to rebase it and solve conflicts with other PRs; it takes time for no reason.

markelog commented 9 years ago

This shouldn't be a blocker for jquery/jquery#1837

mgol commented 9 years ago

@gibson042 comment from https://github.com/jquery/jquery/pull/1837#discussion_r21420605:

I don't know that we've ever established a consensus on this, but I personally see more value in IE<9 (i.e., preserving fix information even for browsers that have rotated out of our support window).

jzaefferer commented 9 years ago

Browser has a bug in version n, it's fixed in n+1: Browser<n+1

Shouldn't this say "up to version n"? Then Browser n would mean "bug in (only) version n".

I'd also always put a space after "Browser', so Browser <n

Browser isn't a great keyword, in jQuery UI we have plenty support comments referring to jQuery versions, like this:

// Support: IE and jQuery <1.9

I'm fine with replacing " and " with ", ".

gnarf commented 9 years ago

So hold on. Is that last example to support IE with jQuery 1.9? Or IE and any other browser in jQuery <1.9? On Dec 8, 2014 8:38 AM, "Jörn Zaefferer" notifications@github.com wrote:

Browser has a bug in version n, it's fixed in n+1: Browser<n+1

Shouldn't this say "up to version n"? Then Browser n would mean "bug in (only) version n".

I'd also always put a space after "Browser', so Browser <n

Browser isn't a great keyword, in jQuery UI we have plenty support comments referring to jQuery versions, like this:

// Support: IE and jQuery <1.9

I'm fine with replacing " and " with ", ".

— Reply to this email directly or view it on GitHub https://github.com/jquery/contribute.jquery.org/issues/95#issuecomment-66116433 .

scottgonzalez commented 9 years ago

In that specific case, I'm not sure. @mikesherov wrote it, so hopefully he knows. It's related to focus and the behavior of .focus() changed in 1.9, so it could go either way.

We do have other examples of support comments that are more complex than just a browser:

// Support: IE<9, Opera in jQuery <1.7 // Support: Voiceover on OS X, JAWS on IE <= 9

gnarf commented 9 years ago

It might be better to never allow two "setups" on the same // Support: line. In that last example of scott's I think it's better to look like:

// Support: IE<9
// Support: Opera in jQuery<1.7
// Support: Voiceover on OS X
// Support: JAWS on IE<=9

This would remove any ambiguity in the last example, and we'd know its only support for IE in jQuery <1.9

scottgonzalez commented 9 years ago

+1 for splitting across multiple support lines.

mgol commented 9 years ago

@jzaefferer

Browser has a bug in version n, it's fixed in n+1: Browser<n+1

Shouldn't this say "up to version n"? Then Browser n would mean "bug in (only) version n".

I assumed only the information I wrote, i.e. we know the bug is in version n and doesn't appear in n+1.

There are interesting cases, though, where a bug doesn't exist in Android 2.3 and >=4.4 but exists in 4.0-4.3. Most of the time we don't check every older one when we already know the workaround will be needed because of a newer browser.

I'd also always put a space after "Browser', so Browser <n

This seems weird to me. I treat both parts as arguments to the < relation so we either should have spaces on both sides or on none. In Core we use both style forms but we've recently started changing to the spaceless one; most of our support comments adhere to that formatting currently. I don't have strict preference here (although maybe the spacey one seems more in line with the general jQuery spacey code style).

mgol commented 9 years ago

@gnarf

It might be better to never allow two "setups" on the same // Support: line.

Good idea.

dmethvin commented 9 years ago

Yeah, :+1: on having only one setup per line. If there is a further explanatory comment for a specific setup, should each follow in a comment line below? e.g.: (totally made up comments)

// Support: IE<9
// Event doesn't bubble properly
// Support: Opera in jQuery<1.7
// Core patch #2421 made this work properly in 1.7
mgol commented 9 years ago

@dmethvin: Yeah, definitely above the comment (that would match current Core comments).

gibson042 commented 9 years ago

Ideally, every bug would have a well-known range. In practice, it's not always expedient or even possible to check old versions, but fortunately those are the ones that matter least. So with the goal of documenting the most possible information, I propose the following:

Introduced＼ Fixed n (immediately follows m) not fixed as of n (latest checked)
in or before k (implied) package<n package<=n+
in or before k (explicit) package<=k-m package<=k-n+
k package k-m package k-n+
m package m only package m-n+ (same as above)
n (meaningless) package n+

e.g., comments for newly-introduced bugs would start on the bottom row and migrate upwards as new versions of the misbehaving software are released/checked and left when the behavior is ultimately fixed.

Comments would not be modified when support is dropped for an old version, unless such rotation obviates the need for a fix entirely.

mgol commented 9 years ago

Ideally, every bug would have a well-known range. In practice, it's not always expedient or even possible to check old versions, but fortunately those are the ones that matter least.

It's useful to have this info if we already know the older version shares a bug, though. MS has been fixing IE11 bugs in patch releases so knowing the issue affects e.g. IE9-11 helps to know the workaround will still be needed even if the issue has been fixed in IE11 (which happens; see https://github.com/jquery/jquery/pull/1901).

gibson042 commented 9 years ago

As for support code targeting more than one piece of software:

// Support: IE<9
// Tolerate nodeList.length returning elements with id "length"
// Support: Safari<6+
// Tolerate nodeList[ number ] returning elements with id equal to the number (jQuery #14142)
gibson042 commented 9 years ago

It's useful to have this info if we already know the older version shares a bug, though. MS has been fixing IE11 bugs in patch releases so knowing the issue affects e.g. IE9-11 helps to know the workaround will still be needed even if the issue has been fixed in IE11 (which happens; see jquery/jquery#1901).

Agreed, which is one reason why I don't want to change support comments when we abandon old browsers.

mgol commented 9 years ago

Agreed, which is one reason why I don't want to change support comments when we abandon old browsers.

I meant that if we support IE9-11 and we just have // Support: IE11 then when IE11 gets fixed, we don't know if the issue targets IE9-10 or not and we can remove the workaround. This problem doesn't exist with regards to browsers that we dropped.

I generally don't mind keeping info about unsupported browsers if the hack is still needed, though, if there are concrete reasons in favor of that. What are your reasons?

gibson042 commented 9 years ago

I meant that if we support IE9-11 and we just have // Support: IE11 then when IE11 gets fixed, we don't know if the issue targets IE9-10 or not and we can remove the workaround. This problem doesn't exist with regards to browsers that we dropped.

But we would know. Support: IE11 would mean that the bug was introduced in IE11 (otherwise it would be e.g. IE 9-11 or IE<12) and fixed in the next version (otherwise it would be IE 11+).

I generally don't mind keeping info about unsupported browsers if the hack is still needed, though, if there are concrete reasons in favor of that. What are your reasons?

  • Elimination of overhead/busy work/yak shaving when abandoning old browsers
  • Accuracy of data in support ranges
  • Possible utility for restoring support, either ourselves (e.g., got the package/range wrong, backporting/cherry-picking a fix, etc.) or in someone's fork extending browser support (e.g., for an intranet project)
scottgonzalez commented 9 years ago

Where do we stand on this? Have @mzgol and @gibson042 come to an agreement?

mgol commented 9 years ago

I fear that // Support: IE10 is a very intuitive thing to write if someone detects a bug in IE10 and this requires them to write // Support: IE10+. When looking at such a comment, I'd never be sure if that means it's fixed in IE11 or if the person just didn't follow the guide. That's why I don't like this particular comment.

dmethvin commented 9 years ago

If the bug still exists in some browser at the time the patch lands, we need some way to indicate that it's still a live bug. The + at the end serves that purpose right? Is there some other proposed method?

mgol commented 9 years ago

@dmethvin Yes, I'm not commenting the + part but that I worry people will omit it when it's needed. We already have loads of comments like // Support: IE8. Having it mean it's fixed in IE9 will mean we suddenly have a lot of comments in the code base that claim the issue is fixed whereas we really don't know it. We can obviously do one large commit adding pluses where needed but I worry people will keep forgetting as Support: IE8 seems to mean the fix is for IE8 and not mean anything else (like that it's fixed in IE9). Avoiding this form altogether will mean that seeing it we'll know it's just an un-updated comment and we need to be careful. Otherwise we may be in doubt constantly.

jaubourg commented 9 years ago

How about:

We then just have to track those // Support comments with no range and no ! at the end at each new browser release and we're good.

It probably highlights a need we'd have for some kind of test page that would reproduce the bugs we have workarounds for. Quite the endeavor though.

dmethvin commented 9 years ago

It seems like we just need to be sure that when we review PRs that we make sure if it's an unresolved bug we use a +. Otherwise the comment is wrong.

gibson042 commented 9 years ago

So we have the following options with respect to Support: <package> <version> style comments:

It's always easy/tempting to skip a + that should really be present, so I'm disavowing support for the first option and have updated https://github.com/jquery/contribute.jquery.org/issues/95#issuecomment-66195679 accordingly. I personally prefer the readable and already-used only to @jaubourg's !, but don't care that much as long as we document it.

dmethvin commented 9 years ago

I'd go for only as well, I think the + is pretty evident but if we're not careful we'll be back to writing Perl.

Not that there's anything wrong with that.

jaubourg commented 9 years ago

only is so much better than !.

I was really making the case for completeness in order to support (pun pun) the entire life-cycle of a support comment. So, obviously, we need 4 formats:

The important thing is how easy it should be to re-evaluate our support comments as we support more browsers.

Let's take two imaginary bugs as an example:

Here is how an imaginary timeline could go and how the support comments would evolve accordingly:

jQuery Browser Bug 1 Comment 1 Bug 2 Comment 2
3.0 IE11 introduced Support: IE11 - -
3.1 IE12 still present Support: IE11+ introduced Support: IE12
3.3 IE13 fixed Support: IE11 to IE12 fixed Support IE12 only

The minutia of the format is non-important to me as long as the format makes it damn easy to understand and maintain the comments.

Hope I make some kind of sense.

mgol commented 9 years ago

@jaubourg We should be explicit; if we change Support: IE11 to Support: IE11+ when we start to know that the bug is still present in IE12 then when IE13 is out we won't know if IE11+ covers IE11-12 or IE11-13. Therefore, we should avoid it. I'd modify your table to the following:

jQuery Browser Bug 1 Comment 1 Bug 2 Comment 2
3.0 IE11 introduced Support: IE11+ - -
3.1 IE12 still present Support: IE11-12+ introduced Support: IE12+
3.3 IE13 fixed Support: IE11-12 fixed Support IE12 only

I mostly like the current version of @gibson042's comment (https://github.com/jquery/contribute.jquery.org/issues/95#issuecomment-66195679) with the exception that we shouldn't write package<n+ as it suggests to mean the same as package<=(n-1)+ which isn't the intention. We can write package k-n+, package<=k-n+ or package<=n+ but never package<n+ IMO.

gibson042 commented 9 years ago

Thanks @mzgol; I agree with the assessment and the point about package<n+. https://github.com/jquery/contribute.jquery.org/issues/95#issuecomment-66195679 updated.

jaubourg commented 9 years ago

@mzgol, the problem with Support: IE11+ right away is that it seems to imply we somehow divined it would also be a problem past IE11 (which we have no way of knowing). Also, using this form doesn't help with people who will undoubtedly forget to put the + sign.

Come to think of it, what would probably work better is not to use the + symbol altogether but use only in a more liberal fashion.

That would transform the table into something like this:

Browser Bug 1 Comment 1 Bug 2 Comment 2
IE11 introduced Support: IE11 - -
IE12 still present Support: IE11-IE12 introduced Support: IE12
IE13 fixed Support: IE11-IE12 only fixed Support: IE12 only

That way, there are now only 2 types of support comments:

Should be much simpler to handle as, now, the only (pun pun) support comments that need re-assessment are those with no only at the end.

What do you guys think?

(also, I favor the full IE11-IE12 form for ranges rather than IE11-12 if only so that searching is easier)

scottgonzalez commented 9 years ago

I like that use of only.

mgol commented 9 years ago

@scottgonzalez @jaubourg I like that too; less ambiguous than what we have now and the most natural comment is the one that asserts the smallest knowledge which is good.

I'm not convinced about repeating the product name, though, like in IE11-IE12. Imagine Firefox20-Firefox23. It's also problematic if the name is not so straigtforward, e.g. Node with jsdom1.5.0-Node with jsdom1.7.0. I'd prefer to write Node with jsdom 1.5.0

BTW, that's also why I prefer to see a space before the version number, so Firefox 20-23 and Node with jsdom 1.5.0-1.7.0.

jaubourg commented 9 years ago

@mzgol, I'd expect we'd have short names for platforms (like FF for Firefox) but I see your point. I'd love to be able to just search for IE11 in my editor/IDE of choice and find all the workarounds for IE11 though and not have to rely on convoluted regexp magic :/

mgol commented 9 years ago

I'd love to be able to just search for IE11 in my editor/IDE of choice and find all the workarounds for IE11

If we had a comment Support: IE10-IE12, you wouldn't find it either. ;) Unless you mean the latest IE, not specifically 11.

jaubourg commented 9 years ago

If we had a comment Support: IE10-IE12, you wouldn't find it either. ;) Unless you mean the latest IE, not specifically 11.

And merry christmas to you too! :(

You're right of course! So the <platform> SPACE <versions> approach makes sense.

arschmitz commented 9 years ago

+1 for SPACE and the most recent suggestion of only

As far as searching while being able to search for a specific version is nice. It would generally only make any sense at all for IE or Safari where there are few major versions and we are very aware of the differences between them and their timeline. FF and Chrome release too often for this to be useful there. Version ranges will typically be more in the range of 26-33 or similar making searching generally useless and I think it would not be very common you would even be wanting to search a specific version on these platforms to begin with.

gibson042 commented 9 years ago

I also like only for known-fixed better than + for unknown. As long as we leave the door open for <= to avoid claiming excess certainty around bug introduction (e.g., Android<=2.3-4.1 only; see https://github.com/jquery/jquery/pull/1837#discussion_r21420605 and https://github.com/jquery/contribute.jquery.org/issues/95#issuecomment-66201399 ), I'm sold.

And still like comma/with/multiple lines for multiple affected packages.

jaubourg commented 9 years ago

As long as we leave the door open for <= to avoid claiming excess certainty around bug introduction (e.g., Android<=2.3-4.1 only; see https://github.com/jquery/jquery/pull/1837#discussion_r21420605 and https://github.com/jquery/contribute.jquery.org/issues/95#issuecomment-66201399 ), I'm sold.

How about Android 2.3?-4.1 only.

If we talk about uncertainty, nothing speaks better than a question mark and it has the added benefit of not shadowing the dash (I find <= a bit noisy but it might just be me).

markelog commented 9 years ago

Didn't follow the thread lately, just read the last message... my hair stand-up.

Will those support comments be turing complete?

mgol commented 9 years ago

How about Android 2.3?-4.1 only.

This form suggests that we don't know if Android 2.3 has the issue whereas @gibson042 wanted to write that we don't know if previous than 2.3 versions share the bug.

jaubourg commented 9 years ago

This form suggests that we don't know if Android 2.3 has the issue whereas @gibson042 wanted to write that we don't know if previous than 2.3 versions share the bug.

I understand, but when I see <=2.3-4.1, I instinctively read <=2.3.4.1 because of the noise the <= introduces. It seems clear to me what x?-y stands for in this context. But I don't care so much I'd throw a tantrum ;)

Will those support comments be turing complete?

Don't you tempt us! :P

markelog commented 9 years ago

:-)

mgol commented 9 years ago

I understand, but when I see <=2.3-4.1, I instinctively read <=2.3.4.1 because of the noise the <= introduces.

We could reduce noise by introducing spaces here. It would also make our comment style guide match our spacey code style. So:

jaubourg commented 9 years ago

Android <= 2.3-4.1

Spacing actually highlights the problem imo. The <= is supposed to apply to the first version in the range, hence my issue with putting it before the whole range. Dunno if you get my visual impairment here ;)

I think we need some way to make clear we actually mean (<=2.3)-4.1... wait would (2.3)-4.1 work?

Anyway, don't let me derail this thread with nitpickery... especially given I just entered the whole discussion so that it could go forward ;P

markelog commented 9 years ago

It seems, this should justify jscs rule, but it looks way to specific to add to jquery preset, perhaps we should create a plugin rule for it? Under jquery org?

gnarf commented 9 years ago

We'd put spaces around a - too... shrug

IE 9 - 11
Android <= 2.3 - 4.1
gibson042 commented 9 years ago 
// Support: Android <=2.3 - 4.1

?

jaubourg commented 9 years ago

Too much spaces... I'll get used to <=2.3-4.1 and we'll be able to close this ;)

scottgonzalez commented 9 years ago

Do we have consensus yet?