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

[Feature] Allow passing environment variables into Playwright.create() #471

Closed
dubinsky opened this issue Jun 7, 2021 · 6 comments · Fixed by #480
Closed

[Feature] Allow passing environment variables into Playwright.create() #471

dubinsky opened this issue Jun 7, 2021 · 6 comments · Fixed by #480

Comments

@dubinsky
Copy link

dubinsky commented Jun 7, 2021

I am embedding Playwright in my Scala code, and need to control browser downloading in code, so I need to set environment variables (PLAYWRIGHT_BROWSERS_PATH, PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD) on the ProcessBuilder used to run the driver.

Since PipeTransport and Connection constructors, Playwright.driverProcess and Playwright.initSharedSelectors() are not public, this results - in addition to duplicating the code of the PlaywrightImpl.create() - in the use of reflection.

It would be cleaner if a flavor of Playwright.create() was added that takes an environment Map<String, String> and delegates
to a similar flavor of PlaywrightImpl.create(), which in turn calls pb.environment().putAll() on the ProcessBuilder...

@dgozman dgozman transferred this issue from microsoft/playwright Jun 7, 2021
@yury-s
Copy link
Member

yury-s commented Jun 7, 2021

The environment variables are inherited from the parent process by default, so if specify PLAYWRIGHT_BROWSERS_PATH and PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD they should be propagated to the driver process, does it not work for you?

@dubinsky
Copy link
Author

dubinsky commented Jun 7, 2021

The environment variables are inherited from the parent process by default, so if specify PLAYWRIGHT_BROWSERS_PATH and PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD they should be propagated to the driver process, does it not work for you?

Environment variables are indeed inherited from the parent process if they are set on the command line, but I want to set them in the code of the parent process itself, and to do that I need to be able to set the environment on the appropriate ProcessBuilder.

@yury-s
Copy link
Member

yury-s commented Jun 7, 2021

Could tell us more about the use case and why setting the variable from the command line is not a viable solution in your case?

@dubinsky
Copy link
Author

dubinsky commented Jun 7, 2021

Could tell us more about the use case and why setting the variable from the command line is not a viable solution in your case?

I am embedding Playwright in the code that will be used (to generate PDFs of web-pages) as a part of s service running in the cloud in one scenario and also as a part of a Gradle plugin in another. I would like to be able to configure Playwright dynamically (in code) depending on the deployment context, and I would really like not to have to expose Playwright-specific environment variables at the top level of my Docker container (and its likes) or force the user of my Gradle plugin to be aware of them and have to set them. With Playwright used as an embedded implementation library, forcing the knowledge of such low-level variables on the user is a breach of encapsulation that I'd rather avoid.

Could you tell me what are the reasons for your apparent reluctance to make Playwright less difficult to embed? Using environment variables to control behavior wouldn't be my first-choice approach, but I think it is reasonable to expect to be able to at least set them from code, without the need to bubble them up all the way to the main entry point of the embedding code (or - worse -to the user thereof)...

@yury-s
Copy link
Member

yury-s commented Jun 9, 2021

Thanks for the detailed explanation!

Could you tell me what are the reasons for your apparent reluctance to make Playwright less difficult to embed? Using environment variables to control behavior wouldn't be my first-choice approach, but I think it is reasonable to expect to be able to at least set them from code, without the need to bubble them up all the way to the main entry point of the embedding code (or - worse -to the user thereof)...

We'd like to keep the API simple and avoid proliferation of number of ways to do the same task unless they are justified by real use cases. We also try to keep the API consistent across the languages and so far env variables alone served us pretty well. We also consider extracting driver/browser installation in a separate step which would likely influence the API you request. With that said I see your point, providing configuration in run time and being able to configure it via Gradle config would make the setup cleaner in some scenarios.

@dubinsky
Copy link
Author

Thank you!

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 a pull request may close this issue.

2 participants