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 Isolated Contexts #1467

Merged
merged 3 commits into from
Aug 29, 2024
Merged

Add Isolated Contexts #1467

merged 3 commits into from
Aug 29, 2024

Conversation

browser-specs-bot
Copy link
Collaborator

Close #1466, adding the suggested spec to the list.

Changes to index.json

This update would trigger the following changes in index.json:

Add spec (1)
{
  "url": "https://wicg.github.io/isolated-web-apps/isolated-contexts.html",
  "seriesComposition": "full",
  "shortname": "isolated-contexts",
  "series": {
    "shortname": "isolated-contexts",
    "currentSpecification": "isolated-contexts",
    "title": "Isolated Contexts",
    "shortTitle": "Isolated Contexts",
    "nightlyUrl": "https://wicg.github.io/isolated-web-apps/isolated-contexts"
  },
  "categories": [],
  "nightly": {
    "url": "https://wicg.github.io/isolated-web-apps/isolated-contexts",
    "status": "Draft Community Group Report",
    "sourcePath": "isolated-contexts.bs",
    "filename": "isolated-contexts.html",
    "alternateUrls": [],
    "repository": "https://github.com/WICG/isolated-web-apps"
  },
  "organization": "W3C",
  "groups": [
    {
      "name": "Web Platform Incubator Community Group",
      "url": "https://www.w3.org/community/wicg/"
    }
  ],
  "title": "Isolated Contexts",
  "source": "specref",
  "shortTitle": "Isolated Contexts",
  "standing": "good"
}

Tests

These changes look good! 😎

Close #1466, adding the suggested spec to the list.

### Changes to `index.json`
This update would trigger the following changes in `index.json`:

<details><summary>Add spec (1)</summary>

```json
{
  "url": "https://wicg.github.io/isolated-web-apps/isolated-contexts.html",
  "seriesComposition": "full",
  "shortname": "isolated-contexts",
  "series": {
    "shortname": "isolated-contexts",
    "currentSpecification": "isolated-contexts",
    "title": "Isolated Contexts",
    "shortTitle": "Isolated Contexts",
    "nightlyUrl": "https://wicg.github.io/isolated-web-apps/isolated-contexts"
  },
  "categories": [],
  "nightly": {
    "url": "https://wicg.github.io/isolated-web-apps/isolated-contexts",
    "status": "Draft Community Group Report",
    "sourcePath": "isolated-contexts.bs",
    "filename": "isolated-contexts.html",
    "alternateUrls": [],
    "repository": "https://github.com/WICG/isolated-web-apps"
  },
  "organization": "W3C",
  "groups": [
    {
      "name": "Web Platform Incubator Community Group",
      "url": "https://www.w3.org/community/wicg/"
    }
  ],
  "title": "Isolated Contexts",
  "source": "specref",
  "shortTitle": "Isolated Contexts",
  "standing": "good"
}
```
</details>

### Tests
These changes look good! 😎
Copy link
Member

@tidoust tidoust left a comment

Choose a reason for hiding this comment

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

The sourcePath property is not needed as far as I can tell. The code uses the spec's shortname and finds isolated-contexts.bs on its own.

The filename property should be useless too... The only reason why it's needed is because the nightly URL does not have the file extension .html. This confuses our code, and may confuse other logic that parses the URL. Also, we haven't integrated that rule into tests, but one of the jobs will create an issue later on to warn us that the base URL of the spec does not match the nightly URL and that we should look into it.

So far, all URLs that target "files" in browser-specs contain the file extension. It would be good if that could be the same for this one. Ideally, that URL would be changed in the spec. In the meantime (or alternatively), the entry could be:

  {
    "categories": [
      "-browser"
    ],
    "shortname": "isolated-contexts",
    "nightly": {
      "url": "https://wicg.github.io/isolated-web-apps/isolated-contexts.html"
    },
    "url": "https://wicg.github.io/isolated-web-apps/isolated-contexts.html"
  }

@robbiemc
Copy link

So far, all URLs that target "files" in browser-specs contain the file extension. It would be good if that could be the same for this one. Ideally, that URL would be changed in the spec. In the meantime (or alternatively), the entry could be:

Thanks for the feedback. Could you elaborate on what changes you're suggesting in the Isolated Context spec?

@tidoust
Copy link
Member

tidoust commented Aug 24, 2024

Thanks for the feedback. Could you elaborate on what changes you're suggesting in the Isolated Context spec?

Sure! Sorry, I was more targeting @dontcallmedom with this comment, who's more intimate with the project and its challenges ;)

So far, in browser-specs, we've only had to deal with two types of URLs:

  1. URLs that target an implicit file in a folder (typically index.html). We normalize these URLs with a final slash in browser-specs (and never add the implicit file, which rather appears under the filename property).
  2. URLs that target a file explicitly. The code assumes that these URLs always include a file extension. Actually, I didn't know that it was possible to drop the extension from a github.io URL and still get the resource.

The immediate problem for browser-specs here is that the code ends up being given a URL that targets a file but that does not specify the file extension. The URL without a file extension appears in the Bikeshed source, at line 7. That's what I was suggesting to fix. But...

Looking into it some more, the actual problem is not there for the time being: browser-specs can fetch info from specs directly but it will use the information in Specref when available (this is done to make sure that tools are aligned). Specref has an entry for Isolated Contexts that reports the URL without the file extension. That entry comes from the WICG biblio file.

Ideally, to get back to a usual situation for browser-specs, both the URL in the spec and the entry in the WICG biblio file should be updated to add .html to the URLs. Now, I don't think that there is anything wrong with dropping the file extension if that's what you prefer. It just looks like a folder URL, and people may be tempted to add a final / out of habit (I see that you fell into that trap when you added the entry, later fixed by Jeffrey).

If you'd prefer to keep the URL without the file extension, supporting such URLs in browser-specs requires us doing a bit of code tweaking, but should be straightforward. The canonical URL used in this pull request would need to drop .html as well in that case.

Copy link
Member

@tidoust tidoust left a comment

Choose a reason for hiding this comment

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

I'm fine with that but I note the URL mismatch with Specref. Now, looking into it, I bumped into WICG/admin#190. That seems a good opportunity to retire WICG's biblio.json file, which would effectively make browser-specs the single source of truth for WICG URLs.

@robbiemc
Copy link

Adding .html to the spec links is fine, I filed PRs against the two repos:
WICG/admin#197
WICG/isolated-web-apps#50

Thanks for explaining the issue!

@tidoust
Copy link
Member

tidoust commented Aug 29, 2024

I'm fine with that but I note the URL mismatch with Specref. Now, looking into it, I bumped into WICG/admin#190. That seems a good opportunity to retire WICG's biblio.json file, which would effectively make browser-specs the single source of truth for WICG URLs.

Specref no longer pulls data from WICG's biblio file. PR to retire the file is at WICG/admin#198.

@tidoust
Copy link
Member

tidoust commented Aug 29, 2024

And since .html was added everywhere, we no longer need to duplicate the URL. I'll update the PR and merge.

URL was updated in the spec and in Specref
@tidoust tidoust merged commit 7f4cb03 into main Aug 29, 2024
1 check passed
@tidoust tidoust deleted the add-isolated-contexts branch August 29, 2024 06:44
@robbiemc
Copy link

Thanks so much for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new spec: Isolated Contexts
4 participants