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 release date for support statements with multiple versions #2995

Merged
merged 2 commits into from
Apr 12, 2021

Conversation

cincodenada
Copy link
Contributor

@cincodenada cincodenada commented Feb 19, 2021

Noticed while I was poking around testing #2966 that entries with additional flagged versions didn't have a release date tooltip. After some digging, I tracked that down to the code that adds in the release date in the first place, which did not account for support statements with multiple entries (for example, a version with flags or a prefix and then a version with full support), so these entries did not have a release date entry in the support data, and thus no tooltip in the compatibility table.

This PR extends the release date population so that it annotates the release date for all versions in an array_support_statement. Happy to get feedback on code structure, the lambda approach to this kind of thing is certainly of a style and I'm not sure if it's in line with the rest of the project. A functionally equivalent approach would be as follows, I went with the lambda because this one seemed less obviously safe in regards to object references/copying, but I tested it and it does indeed work just as well:

      for (const [browser, info] of Object.entries(block.support)) {
        const infoEntries = Array.isArray(info) ? info : [info]
        for (const infoEntry of infoEntries) {
          const added = infoEntry.version_added;
          if (browserReleaseData.has(browser)) {
            if (browserReleaseData.get(browser).has(added)) {
              infoEntry.release_date = browserReleaseData
                .get(browser)
                .get(added).release_date;
            }
          }
        }
      }

Copy link
Contributor

@peterbe peterbe left a comment

Choose a reason for hiding this comment

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

I love the feature but not in love with the implementation.
First of all, I think it's bad to generate a new function for every iteration in a loop.
You could solve that by simply moving the closure function (you call it a "lambda" function) up outside the for (const [key, compat] of Object.entries(data)) { loop.

However, I think a much neater and more correct pattern would be to meet the data where it's flawed instead. I'm a bit surprised about the fact that block.support yields arrays sometimes and objects sometimes. Perhaps I've noticed this before. Perhaps it wasn't meant to be like that but the powerful nature of change caused this to happen as the project matured. What I would prefer is this:

for (const [browser, info] of Object.entries(block.support)) {
  // THAT wonderful code comment you already wrote here
  if (!Array.isArray(info)) {
    info = [info];
  }
  // WIth the oddity taken care of, from here on we know we can always loop over the `info`
  // entries.
  for (const infoEntry of info) {
            addReleaseDate(browser, infoEntry);
          }

Would you mind look into that refactoring?

@peterbe
Copy link
Contributor

peterbe commented Feb 22, 2021

By the way. I really like the functionality change.

@peterbe
Copy link
Contributor

peterbe commented Feb 22, 2021

CC @ddbeck soon we'll have much better tooltips about the release dates on entries where there's more than just one version "event". This @cincodenada is on fire!

@ddbeck
Copy link
Contributor

ddbeck commented Feb 23, 2021

This is exciting!

meet the data where it's flawed

Yeah, this is a quirk of BCD's schema. A browser in a support object can be either an array of support statements (e.g., "firefox": [ {"version_added": "…" }, { "version_added": "…", "version_removed": "…" } ]) or a single support statement alone (e.g., "firefox": { "version_added": "…" }). We've never actually done anything about it because it's not a quick fix and nobody's filed an issue about it. I'll let you do with that information as you will.

@cincodenada
Copy link
Contributor Author

cincodenada commented Feb 27, 2021

Okay great, I've reworked things to meet the data where it's at and get it into a single consistent form, and looks to work just as well indeed.

@peterbe
Copy link
Contributor

peterbe commented Mar 1, 2021

🍾 🚀 Well done! And thank you!
Do you think you could help me by testing #2992?

@peterbe
Copy link
Contributor

peterbe commented Mar 1, 2021

@cincodenada I think there's a bug in GitHub right now. I can't merge it. Your commits need to be signed and that option to override and ignore that (because I'm admin) isn't appearing.
To merge this, do you think you can rebase it with a signed commit?

@Ryuno-Ki
Copy link

Ryuno-Ki commented Mar 1, 2021

I think there's a bug in GitHub right now.

Earlier, GitHub reported a degradation of their GitHub Actions services.

@cincodenada
Copy link
Contributor Author

I should probably get commit signing set up anyway, so I'll do that, hopefully tonight, and rebase when I do 👍

@cincodenada cincodenada force-pushed the release-date-multiple-entries branch from 91b5123 to aa5e832 Compare April 3, 2021 21:22
The code that added in the release date did not account for support
statements that involve multiple releases (for example, a version with
flags and then a version with full support), so these entries did not
have a release date tooltip in the compatibility table.

This commit extends the release date addition to annotate the release
date for all versions in a support statement.
Just ensure we always are dealing with an array
@cincodenada cincodenada force-pushed the release-date-multiple-entries branch from aa5e832 to 39be0fe Compare April 3, 2021 21:24
@cincodenada
Copy link
Contributor Author

Okay, apologies for the absurd delay here, but I finally managed to focus long enough to sit down and get a key generated and figure out how to sign my commits. I've force-pushed the signed commits to my branch, and rebased on main and squashed it down to two commits while I was at it.

Let me know if things look good on your end, and thanks again!

@peterbe
Copy link
Contributor

peterbe commented Apr 12, 2021

Wohoo!!! Thanks @cincodenada !

peterbe pushed a commit to peterbe/yari that referenced this pull request Jun 1, 2021
)

* Add release date for support statements with multiple versions

The code that added in the release date did not account for support
statements that involve multiple releases (for example, a version with
flags and then a version with full support), so these entries did not
have a release date tooltip in the compatibility table.

This commit extends the release date addition to annotate the release
date for all versions in a support statement.

* Standardize the data, don't deal with two forms

Just ensure we always are dealing with an array
@caugner caugner added the browser-compat issues related to the browser compatibility data tables (BCD) label Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser-compat issues related to the browser compatibility data tables (BCD)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants