feat(respecDocWriter): use locally installed respec version #2957
Conversation
sidvishnoi
requested review from
saschanaz and
marcoscaceres
and removed request for
saschanaz and
marcoscaceres
| timeout = 300000, | ||
| disableSandbox = false, | ||
| debug = false, | ||
| useLocal = false, |
There was a problem hiding this comment.
Should this be a string/boolean instead of just boolean? Having a string could allow using a custom URL (say, from unpkg) instead of using local one.
There was a problem hiding this comment.
I don't have a strong opinion. Whatever you think is best... or leave it for now, and add the custom URL when we need it.
| return path.startsWith("/respec/builds/"); | ||
| default: | ||
| // localhost, file://, and everything else | ||
| return /\/builds\/respec-[\w-]+\.js$/.test(path); |
There was a problem hiding this comment.
Why not just respec-[\w-]+\.js?
There was a problem hiding this comment.
Could also match respec-worker, respec-highlight
There was a problem hiding this comment.
Yes, I mean why builds/?
There was a problem hiding this comment.
We're only replacing respec-[profile] here, which is in builds/. Let me try if we can do without /builds/
There was a problem hiding this comment.
Adding /builds/ reduces risk of false-positive though. A mismatch here means crash. Say, some adds their config as respec-config.js (haven't seen cases, but possible). builds/respec-config.js is less likely.
There was a problem hiding this comment.
Download from cache is kinda slow though. Given we already have a browser installed, it'll definitely feel slower to not use it.
PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD=1 should let us skip download though.
Nice find on passing executable path. Can set it with env variables.
I'll play more with it, if we do want to go with playwright. Though, I'm okay with puppeteer also.
There was a problem hiding this comment.
It's microsoft/playwright#5952 vs puppeteer/puppeteer#4208 now.
There was a problem hiding this comment.
@saschanaz I think intercepting only builds/respec-{profile} should be good enough for now?
There was a problem hiding this comment.
Yeah, it seems it won't be fixed anytime soon.
There was a problem hiding this comment.
@saschanaz importScripts from workers should now be fixed on vNext/master of playwright. When v1.11.0 comes out, feel free to give it a try and raise an issue if you encounter problems. 😄
NB: This fix should simply prevent the importScripts from hanging; I'm not sure of the general story for request interception of SW traffic.
sidvishnoi
force-pushed
the
versioned-respecDocWriter
branch
from
6967e73
to
4bd9ca3
Compare
Closes https://github.com/w3c/respec/issues/2874