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

Document style of "support" comments #95

Open
markelog opened this issue Dec 7, 2014 · 81 comments
Open

Document style of "support" comments #95

markelog opened this issue Dec 7, 2014 · 81 comments

Comments

@markelog
Copy link
Member

markelog commented Dec 7, 2014

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
Copy link
Member

mgol commented Dec 8, 2014

👍 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
Copy link
Member

mgol commented Dec 8, 2014

BTW, if we don't come up to a concrete conclusion quickly I'd like to merge jquery/jquery#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
Copy link
Member Author

markelog commented Dec 8, 2014

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

@mgol
Copy link
Member

mgol commented Dec 8, 2014

@gibson042 comment from jquery/jquery#1837 (comment):

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
Copy link
Member

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
Copy link
Member

gnarf commented Dec 8, 2014

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" [email protected] 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
#95 (comment)
.

@scottgonzalez
Copy link
Member

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
Copy link
Member

gnarf commented Dec 8, 2014

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
Copy link
Member

+1 for splitting across multiple support lines.

@mgol
Copy link
Member

mgol commented Dec 8, 2014

@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
Copy link
Member

mgol commented Dec 8, 2014

@gnarf

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

Good idea.

@dmethvin
Copy link
Member

dmethvin commented Dec 8, 2014

Yeah, 👍 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
Copy link
Member

mgol commented Dec 8, 2014

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

gibson042 referenced this issue in jquery/jquery Dec 8, 2014
That includes IE<8, Opera 12.x, Firefox<29, Safari<6.0 and some hacks
for old Blackberry.

Fixes gh-1836
Fixes gh-1701
Refs gh-1815
Refs gh-1820
@gibson042
Copy link
Member

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
Copy link
Member

mgol commented Dec 8, 2014

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 jquery/jquery#1901).

@gibson042
Copy link
Member

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
Copy link
Member

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
Copy link
Member

mgol commented Dec 8, 2014

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
Copy link
Member

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
Copy link
Member

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

@mgol
Copy link
Member

mgol commented Dec 19, 2014

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
Copy link
Member

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
Copy link
Member

mgol commented Dec 19, 2014

@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
Copy link
Member

How about:

  • // Support: IE8! (or whatever symbol that doesn't look too weird) when there is a higher version and we know and checked it's only IE8 (or // Support: IE8-IE9 for ranges),
  • // Support: IE8+ when we know it's still a bug in higher versions,
  • // Support: IE8 if we don't know or there is no higher version (what people would put by default anyway).

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
Copy link
Member

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
Copy link
Member

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 #95 (comment) 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
Copy link
Member

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
Copy link
Member

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:

  • <package> <version>: bug was introduced in version which happens to be the latest version we support,
  • <package> <version> +: bug was introduced in version and following version(s) still exhibit(s) it,
  • <package> <version> only: bug was present in version and following version(s) do(es) no exhibit it,
  • <package> <version1> to <version2>: bug was introduced in version1, present up to version2 and has been fixed then.

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:

  • Bug1, introduced in IE11 and fixed in IE13,
  • Bug2, introduced in IE12 and also fixed in IE13.

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
Copy link
Member

mgol commented Sep 28, 2015

In light of new info at jquery/testswarm#308 (comment) we should use Edge 12 only, Edge 12-13+ etc.

mgol added a commit to mgol/jquery that referenced this issue Mar 23, 2016
mgol added a commit to mgol/jquery that referenced this issue Mar 23, 2016
mgol added a commit to mgol/jquery that referenced this issue Mar 23, 2016
mgol added a commit to mgol/jquery that referenced this issue Mar 23, 2016
mgol added a commit to mgol/jquery that referenced this issue Mar 30, 2016
mgol added a commit to mgol/jquery that referenced this issue Mar 30, 2016
mgol added a commit to mgol/jquery that referenced this issue Mar 30, 2016
mgol added a commit to mgol/jquery that referenced this issue Mar 30, 2016
@mgol
Copy link
Member

mgol commented Dec 20, 2018

I've been thinking about that and I can't get rid of the feeling that syntax like:

Firefox <=35-42

suggests the issue only appears in versions 42 or older. I think it'd be better to not use this form at all and keep:

Chrome <=45-50 only

but at the same time for unclosed ranges use:

Firefox <=35-42+

What do you think? cc @gibson042 @timmywil @dmethvin @markelog @jbedard

@gibson042
Copy link
Member

Works for me. It reads a little strangely when the "plus" version doesn't exist, but remains comprehensible.

@jbedard
Copy link

jbedard commented Dec 21, 2018

I agree Firefox <=35-42 looks like the issue no longer exists in 43+ and would suggest deleting something once 42 is no longer supported. Your suggestion SGTM 👍

mgol added a commit to mgol/jquery that referenced this issue Apr 10, 2019
Also, update support comments format to match format described in:
jquery/contribute.jquery.org#95 (comment)
with the change from:
jquery/contribute.jquery.org#95 (comment)
(open-ended ranges end with `+`).

Further size reductions will be achieved when we drop Firefox 60, iOS 10
and pre-Chromium Edge versions.

Fixes jquerygh-3950
Fixes jquerygh-4299
mgol added a commit to mgol/jquery that referenced this issue Apr 10, 2019
Also, update support comments format to match format described in:
jquery/contribute.jquery.org#95 (comment)
with the change from:
jquery/contribute.jquery.org#95 (comment)
(open-ended ranges end with `+`).

Further size reductions will be achieved when we drop Firefox 60, iOS 10
and pre-Chromium Edge versions.

Fixes jquerygh-3950
Fixes jquerygh-4299
mgol added a commit to mgol/jquery that referenced this issue Apr 10, 2019
Also, update support comments format to match format described in:
jquery/contribute.jquery.org#95 (comment)
with the change from:
jquery/contribute.jquery.org#95 (comment)
(open-ended ranges end with `+`).

Further size reductions will be achieved when we drop Firefox 60, iOS 10
and pre-Chromium Edge versions.

Fixes jquerygh-3950
Fixes jquerygh-4299
mgol added a commit to mgol/jquery that referenced this issue Apr 13, 2019
Also, update support comments format to match format described in:
jquery/contribute.jquery.org#95 (comment)
with the change from:
jquery/contribute.jquery.org#95 (comment)
(open-ended ranges end with `+`).

Further size reductions will be achieved when we drop Firefox 60, iOS 10
and pre-Chromium Edge versions.

Fixes jquerygh-3950
Fixes jquerygh-4299
mgol added a commit to mgol/jquery that referenced this issue Apr 15, 2019
Also, update support comments format to match format described in:
jquery/contribute.jquery.org#95 (comment)
with the change from:
jquery/contribute.jquery.org#95 (comment)
(open-ended ranges end with `+`).

Further size reductions will be achieved when we drop Firefox 60, iOS 10
and pre-Chromium Edge versions.

Fixes jquerygh-3950
Fixes jquerygh-4299
mgol added a commit to mgol/jquery that referenced this issue Apr 17, 2019
Also, update support comments format to match format described in:
jquery/contribute.jquery.org#95 (comment)
with the change from:
jquery/contribute.jquery.org#95 (comment)
(open-ended ranges end with `+`).

Further size reductions will be achieved when we drop Firefox 60, iOS 10
and pre-Chromium Edge versions.

Fixes jquerygh-3950
Fixes jquerygh-4299
mgol added a commit to mgol/jquery that referenced this issue Apr 23, 2019
Also, update support comments format to match format described in:
jquery/contribute.jquery.org#95 (comment)
with the change from:
jquery/contribute.jquery.org#95 (comment)
(open-ended ranges end with `+`).

Further size reductions will be achieved when we drop Firefox 60, iOS 10
and pre-Chromium Edge versions.

Fixes jquerygh-3950
Fixes jquerygh-4299
mgol added a commit to mgol/jquery that referenced this issue Apr 29, 2019
…& PhantomJS

Also, update support comments format to match format described in:
jquery/contribute.jquery.org#95 (comment)
with the change from:
jquery/contribute.jquery.org#95 (comment)
(open-ended ranges end with `+`).

Fixes jquerygh-3950
Fixes jquerygh-4299
mgol added a commit to jquery/jquery that referenced this issue Apr 29, 2019
…& PhantomJS

Also, update support comments format to match format described in:
jquery/contribute.jquery.org#95 (comment)
with the change from:
jquery/contribute.jquery.org#95 (comment)
(open-ended ranges end with `+`).

Fixes gh-3950
Fixes gh-4299
Closes gh-4347
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests