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

feat: support ignoreHTTPSErrors when Playwright browser opens a new page #531

Merged
merged 2 commits into from
Oct 21, 2024

Conversation

Mura-Mi
Copy link
Contributor

@Mura-Mi Mura-Mi commented Oct 11, 2024

about

fix #530

Playwright の Browser が newPage する際のオプションに ignoreHTTPSErrors をCLIフラグから設定できるようにしました。
環境依存の属性なので build コマンド及び preview コマンドの CLI オプションのみから設定できるものとし、 vivliostyle.config.js の設定項目には加えていません。

動作確認

パブリックインターネットへのアクセスに自己署名証明書を利用する必要のある環境にて、--ignore-https-errors オプションを付して数式記法を含む markdown ドキュメントをビルドし、レンダリング結果に mathjax 記法による数式レンダリングが正常になされていることを確認。

@spring-raining
Copy link
Member

@Mura-Mi PRありがとうございます! 以下の点をご確認お願いします

  • こちらのPRをテストしようとしていますが、Support ignoreHTTPSErrors option for Playwright Browser#newPage #530 の内容を察するに --ignore-https-errors オプションに加えて Mura-Mi:f/proxy で実装されている --proxy-server も同時に使用することを前提とされているようなので、このPRに Mura-Mi:f/proxy の変更も加えてもらえないでしょうか?
  • 簡単で良いので変更した機能の検証手順をdescriptionに追加してもらえますか?

husky から発火するドキュメント自動生成周りで大きめの差分が出ているのですが原因がわからず、もし対処する必要があればご教示いただけると助かります。

すみません、こちらの問題は修正しましたので現時点でのmain branchをrebaseしてご使用ください。 ref: #532

@Mura-Mi
Copy link
Contributor Author

Mura-Mi commented Oct 15, 2024

@spring-raining 確認ありがとうございます。
Proxy 関連は、私の環境では特に設定不要だったのですが、確かに間違いなく ignore https errors と合わせて使うケースが多そうですね。承知しました。
#339 では、Playwright が起動する Chromium の起動引数プロキシ情報を追加するように書いてありますが、https://playwright.dev/docs/api/class-browser#browser-new-context-option-proxy で Playwright のプロキシ設定をするようなオプションもあります。他ブラウザの対応などを想定してもこちらで実装したほうが好ましいかと思いますが、いかがでしょうか。

desc の記載、rebase なども対応します 👍

@Mura-Mi Mura-Mi force-pushed the f/ignore-https-erros branch from d7b104f to 23ed255 Compare October 17, 2024 15:54
@Mura-Mi
Copy link
Contributor Author

Mura-Mi commented Oct 17, 2024

@spring-raining 検証手順の記載、プロキシ設定の追加、ブランチのりベースを行いました。
プロキシ設定に関しては #339 にて認証付きプロキシのリクエストがあったために username/password を CLI フラグから指定する方法も追加しました。
また、プロキシ設定は HTTP_PROXY および NO_PROXY環境変数に設定されることもあるため、そのパターンも考慮しています(この場合、認証付きプロキシを通す場合は HTTP_PROXY に username/password を含める形になることを想定しています。むしろ cli から設定するときも username・password 用のフラグを用意しなくてもよいのでは?とも思いましたが、Playwright の proxy オプションとシグニチャを合わせる方を取っています)

ご確認をお願いします!

@@ -7,6 +7,7 @@ try {
const program = setupBuildParserProgram();
program.parse(process.argv);
const options = program.opts();
console.log(options);
Copy link
Member

Choose a reason for hiding this comment

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

こちらの行の削除をお願いします!

Suggested change
console.log(options);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

失礼しました!削除したうえでプロキシ対応のコミットをsquashしてあります。

@Mura-Mi Mura-Mi force-pushed the f/ignore-https-erros branch from 23ed255 to c8f2c69 Compare October 19, 2024 06:22
Copy link
Member

@spring-raining spring-raining left a comment

Choose a reason for hiding this comment

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

良さそうです。ありがとうございます!

@spring-raining spring-raining merged commit 628d1dd into vivliostyle:main Oct 21, 2024
1 check passed
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.

Support ignoreHTTPSErrors option for Playwright Browser#newPage
2 participants