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

Remove service_workers_support from api/FileReaderSync #8014

Merged
merged 5 commits into from
Jan 18, 2021

Conversation

hamishwillee
Copy link
Collaborator

There is a Worker support guideline so service_workers_support is incorrect. This follows on from this discussion: #7849 (comment)

@ddbeck The way I have done this is to just add a note indicating that service workers are not supported (with the version, if known).

Are there other alternatives? i.e. should/can I do multiple worker supports like this?:

 "worker_support": {
        "__compat": {
          "description": "Available in workers",
...

 "worker_support": {
        "__compat": {
          "description": "Available in service workers",
...

Or something like this (feels confusing).

 "worker_support": {
        "__compat": {
          "description": "Available in workers",
          "support": {
            "chrome": [
              {
                "version_added": true,
              },
              {
                "version_added": true,
                "version_removed": "59",
                "notes": "Service workers not supported from Chrome 59"
              },
            ],

@github-actions github-actions bot added the data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Dec 18, 2020
@hamishwillee
Copy link
Collaborator Author

Note, same question for caches. In chrome, for example, support was added for service workers in version 40. In version 43 support was added for other workers.

So I what is the right way to do this - added in version 40, and a note stating that initially only for service workers. Or two worker_support lines, or one worker support line but with notes about what happened in each version?

@ddbeck
Copy link
Collaborator

ddbeck commented Dec 18, 2020

It's hard to follow the diff for removals and data changes in one commit. Can you put into words the support story you're trying to tell here? The diff and the examples given seem to be telling competing stories. What was supported (and unsupported) and in what versions did support change?

That said, I can rule out one thing: multiple "worker_support" features. JSON keys have to be unique, or else we'll just clobber one bit of data with another.

Data like this also seems unlikely:

            "chrome": [
              {
                "version_added": true,
              },
              {
                "version_added": true,
                "version_removed": "59",
                "notes": "Service workers not supported from Chrome 59"
              },
            ],

The second support statement says support was dropped in version 59. The note is a bit strange, since it reiterates what "version_removed": "59" is already saying: from version 59, the feature is unsupported (though the first support statement says that support was reintroduced in some unknown version).

@hamishwillee
Copy link
Collaborator Author

Can you put into words the support story you're trying to tell here?

Hi @ddbeck
All I have done (see diff) is deleted the service_workers_support key and captured the version information about service worker support in a note under worker_support.

So to answer your question, this is supposed to say:

  • workers, other than service workers are supported
  • Info of versions in which there is support for workers
  • Info about versions in which there was support for service workers.

This is probably OK for this case but I'm not sure it is the "right way". How would YOU present for chrome this imaginary case.

  • dedicated workers are supported from version 4,
  • shared workers are supported from version 8
  • service workers are supported from version 2 to 6.
  • chrome workers are not supported

@ddbeck ddbeck self-requested a review January 7, 2021 13:22
Copy link
Collaborator

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

Thanks for the update, @hamishwillee.

First, to answer your hypothetical:

How would YOU present for chrome this imaginary case.

  • dedicated workers are supported from version 4,
  • shared workers are supported from version 8
  • service workers are supported from version 2 to 6.
  • chrome workers are not supported

Right now, given our schema, probably with something like this:

{
  "version_added": "2",
  "notes": [
    "From version 8, shared workers are supported.",
    "From version 4, dedicated workers are supported.",
    "From version 2 to 6, service workers are supported; from version 7, service workers are unsupported."
  ]
}

Hypothetical aside, the new diff here makes a lot more sense to me. For example, in Chrome supported initially and generally, then dropped from service workers specifically. Suggests in line comments are just some style and consistency nits.

Also, just so I'm doing my due diligence here: I take it you did some sort of testing to figure out the support data here?

api/FileReaderSync.json Outdated Show resolved Hide resolved
api/FileReaderSync.json Outdated Show resolved Hide resolved
api/FileReaderSync.json Outdated Show resolved Hide resolved
api/FileReaderSync.json Outdated Show resolved Hide resolved
api/FileReaderSync.json Outdated Show resolved Hide resolved
api/FileReaderSync.json Outdated Show resolved Hide resolved
api/FileReaderSync.json Outdated Show resolved Hide resolved
api/FileReaderSync.json Outdated Show resolved Hide resolved
api/FileReaderSync.json Outdated Show resolved Hide resolved
api/FileReaderSync.json Outdated Show resolved Hide resolved
hamishwillee and others added 2 commits January 17, 2021 18:51
Co-authored-by: Daniel D. Beck <daniel@ddbeck.com>
@hamishwillee
Copy link
Collaborator Author

Hypothetical aside, the new diff here makes a lot more sense to me. For example, in Chrome supported initially and generally, then dropped from service workers specifically. Suggests in line comments are just some style and consistency nits.

Agree - what you have suggested makes sense for the current data.

NOTE, that support for workers implies support for dedicated workers, shared workers and service workers. It does not imply support for the {FF-only) chrome workers - which I propose we completely ignore.

Also, just so I'm doing my due diligence here: I take it you did some sort of testing to figure out the support data here?

No. There is no new data in this change - it is just a reformatting of the data that was already present.

Copy link
Collaborator

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you! 🎉

@ddbeck ddbeck merged commit 3714a28 into mdn:master Jan 18, 2021
@hamishwillee hamishwillee deleted the FileReaderSync_Workers branch January 18, 2021 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants