feat(respec2html): intercept and replace ReSpec with in-tree build #2702
Conversation
|
This is in a draft state, but I want feedback so submitting as a non-draft PR. The purpose of this would be to avoid a repeat of what happened in w3c/reffy#205 by pinning the version of ReSpec used by Reffy. This approach isn't perfect as ReSpec could be served from elsewhere, most plausibly a local copy for the spec. Is there a better way to achieve the same goal? If this approach is generally reasonable, should it be behind a flag and how should it be tested? |
0e2d1c6 to
708269e
Compare
| // Intercept requests for ReSpec and serve it from the local build instead. | ||
| // https://github.com/tidoust/reffy/pull/212 | ||
| await page.setRequestInterception(true); | ||
| page.on("request", async function callback(request) { |
There was a problem hiding this comment.
@sidvishnoi, I believe your netlify work does similar work to load the spec with a specified build, am I right?
There was a problem hiding this comment.
Yes, but in a service worker. https://respec-preview.netlify.com/
There was a problem hiding this comment.
Is that https://github.com/w3c/respec/blob/develop/tools/netlify.js? Where's the logic to replace the ReSpec version?
Are there changes to ReSpec itself that could be made to make this kind of replacing more robust? I'm thinking maybe having the script load but do nothing, or post a message saying "please inject your own ReSpec now"?
There was a problem hiding this comment.
Using a regex to replace commonly used ReSpec scripts: https://github.com/sidvishnoi/respec-preview/blob/master/sw.js
There was a problem hiding this comment.
I see, that's not far from what I'm doing, both hostname and the first part of the pathname need to match exactly. My version would also match anything else that's in the build/ directory, but I don't know that it's needed. The code I have now would only replace the first request so I expect it's actually broken in some cases.
While it certainly wasn't a good experience with ReSpec changes breaking Reffy, I'm not sure this approach of locking down ReSpec version is good idea. Do you plan to lock down to a major version or minor version? If still desired, this change should be behind a flag though. |
I think this should be by default, as that's what we expect for a versioned package. Ignoring its own version by default is a weird behavior. This is one of my purpose in #2187, although no recent progress there. |
The version used will be bumped automatically with PRs like w3c/reffy#216, so it'll be updated continuously. Like most version pinning, the purpose would be to have control over when the dependency changes, so that one can deal with the change before it affects the master branch / production. That being said, I'd defer to @tidoust on how important it is to pin the ReSpec version. |
|
Makes sense. I was concerned as specs are using latest versions, and if tools are using a different version than the spec, it might lead to inconsistencies. For example, we made some change in ReSpec and the spec reflected it (by changing markup etc.), but it was incompatible with ReSpec version being used at Reffy and other tools. |
|
Specs that are using https://www.w3.org/Tools/respec/respec-w3c are betting that there won't be any backwards incompatible changes, but may of course be quick to use new features once released. I suspect that the risk of breakage due to that is pretty low, however, as long as updates in Reffy lag behind only a few days. |
|
Tried to make tests pass and simplified profile finding logic a bit // Intercept requests for ReSpec and serve it from the local build instead.
// https://github.com/tidoust/reffy/pull/212
if (!new URL(src).hostname.includes("localhost")) { // this line prevents tests:headless from failing
await page.setRequestInterception(true);
const respecProfileRegex = /((?:respec-w3c)(?:-common)?)/;
/** @param {URL} url */
const isRespecRequest = url => {
return (
url.hostname.includes("w3.org") &&
respecProfileRegex.test(url.pathname)
);
};
page.on("request", async function requestInterceptor(request) {
const url = new URL(request.url());
if (!isRespecRequest(url)) {
await request.continue();
return;
}
const profile = url.pathname.match(respecProfileRegex)[1];
const localPath = path.join(__dirname, "..", "builds", `${profile}.js`);
console.log(`Intercepted ${url} to respond with ${localPath}`);
await request.respond({
contentType: "text/javascript; charset=utf-8",
body: await readFile(localPath),
});
// Workaround for https://github.com/puppeteer/puppeteer/issues/4208
page.removeListener("request", requestInterceptor);
await page.setRequestInterception(false);
});
} |
|
@foolip noted:
Ah. I don't really know :) In the short term, that seems useful to avoid situations where crawls generate invalid PRs against Web Platform Tests. I don't know what the right strategy is in the long run. That is, it might be better to check assumptions that Reffy has regarding document structure (e.g. how to find WebIDL extracts or identify informative sections in a ReSpec spec, how to identify informative sections, etc.) before a crawl runs and prevent PRs from being created when something breaks, instead of pinning down the version of ReSpec which requires a manual intervention each time there is a ReSpec update. And/Or, and that's perhaps even easier, we could simply run the WebIDL parser on the extracted WebIDL in Reffy (which we actually already do) and only submit PRs to Web Platform Tests when parsing succeeds. As a side note, Reffy may stop using respecDocWriter altogether at some point: Reffy parses non ReSpec specs as well and since respecDocWriter needs Puppeteer, we could just as well use Puppeeter throughout and run our extraction logic there directly (instead of relying on JSDOM). That would make processing less convoluted and allow us to drop a few dependencies. I'd grab the code proposed here if we do that. |
|
Thanks for the discussion everyone. It sounds from @tidoust like Reffy will likely move away from using this tool in favor of loading the spec in Puppeteer directly, in which case any support I add here will be unused. I'm going to close this, but if anyone else using respec2html would like it, then you're free to cherry pick and continue working on it. |
No description provided.