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.

Sign up for GitHub

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

Jump to bottom

feat(respecDocWriter): use locally installed respec version #2957

Merged
merged 2 commits into from Mar 31, 2021
Merged

feat(respecDocWriter): use locally installed respec version #2957

merged 2 commits into from Mar 31, 2021

Conversation

sidvishnoi
Copy link
Member

@sidvishnoi sidvishnoi commented Jun 12, 2020 •
edited

Closes #2874

# use the same version as in document
respec2html --src examples/basic.built.html -o /dev/null

# use the local version instead 🎉 
respec2html --src examples/basic.built.html -o /dev/null --use-local

@sidvishnoi sidvishnoi requested review from saschanaz and marcoscaceres and removed request for saschanaz and marcoscaceres June 12, 2020 21:41
@sidvishnoi sidvishnoi marked this pull request as ready for review June 12, 2020 21:58
sidvishnoi
sidvishnoi commented Jun 12, 2020
View reviewed changes
tools/respecDocWriter.js
timeout = 300000,
disableSandbox = false,
debug = false,
useLocal = false,
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

saschanaz
saschanaz reviewed Jun 12, 2020
View reviewed changes
tests/headless.js Outdated Show resolved Hide resolved
tools/respecDocWriter.js
return path.startsWith("/respec/builds/");
default:
// localhost, file://, and everything else
return /\/builds\/respec-[\w-]+\.js$/.test(path);
Copy link
Member

Choose a reason for hiding this comment

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

Why not just respec-[\w-]+\.js?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could also match respec-worker, respec-highlight

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I mean why builds/?

Copy link
Member Author

Choose a reason for hiding this comment

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

We're only replacing respec-[profile] here, which is in builds/. Let me try if we can do without /builds/

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's microsoft/playwright#5952 vs puppeteer/puppeteer#4208 now.

Copy link
Member Author

Choose a reason for hiding this comment

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

@saschanaz I think intercepting only builds/respec-{profile} should be good enough for now?

Copy link
Member

@saschanaz saschanaz Mar 29, 2021 •
edited

Choose a reason for hiding this comment

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

Yeah, it seems it won't be fixed anytime soon.

Copy link

Choose a reason for hiding this comment

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

@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.

marcoscaceres
marcoscaceres approved these changes Jun 17, 2020
View reviewed changes
Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@sidvishnoi sidvishnoi added this to In progress in Up next Jun 26, 2020
@sidvishnoi sidvishnoi removed this from In progress in Up next Jun 26, 2020
marcoscaceres
marcoscaceres approved these changes Jul 21, 2020
View reviewed changes
Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

LGTM

@sidvishnoi sidvishnoi merged commit c301fb8 into develop Mar 31, 2021
@sidvishnoi sidvishnoi deleted the versioned-respecDocWriter branch March 31, 2021 05:17
@sidvishnoi sidvishnoi mentioned this pull request Mar 31, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saschanaz saschanaz saschanaz left review comments

@rwoll rwoll rwoll left review comments

@marcoscaceres marcoscaceres marcoscaceres approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use custom ReSpec version while generating via respec2html
4 participants
@sidvishnoi @marcoscaceres @saschanaz @rwoll