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

[BUG] Cannot override .npmrc env var PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD from true to false #4225

Closed
majapw opened this issue Oct 23, 2020 · 6 comments

Comments

@majapw
Copy link
Contributor

majapw commented Oct 23, 2020

Context:

  • Playwright Version: 1.5.1
  • Operating System: Linux for CI, Mac locally
  • Node.js version: 12.16.2
  • Browser: All

Describe the bug

The scenario we have is that we want to skip installing browsers by default in our ecosystem, but manually install browsers if a person is running playwright code locally or as part of the CI job that runs playwright code. As a result we set the following in our .npmrc:

playwright_skip_browser_download=1

Unfortunately, PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD="" yarn does not override this value (or any variation of this). I think this is because the concept of overriding env vars in the playwright repo is predicated on the idea of overriding falsey values with truthy values and not the other way around.

If we look at the code to skip the browser install:

  if (getFromENV('PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD')) {
    browserFetcher.logPolitely('Skipping browsers download because `PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD` env variable is set');
    return false;
  }
``
We need `getFromENV('PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD')` to return a falsey value to get past this block. When we look at the code from [`getFromEnv`](https://github.com/microsoft/playwright/blob/master/src/utils/utils.ts#L98-L103):

export function getFromENV(name: string) {
let value = process.env[name];
value = value || process.env[npm_config_${name.toLowerCase()}];
value = value || process.env[npm_package_config_${name.toLowerCase()}];
return value;
}

we see that it's impossible to override the truthy value of `process.env.npm_config_playwright_skip_browser_download` with a falsey value no matter what we do. I think changes the `||` operators to `??` might fix the issue.

Additionally, it might be worth being explicit about possible values for `PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD`, cast them from a string to a number (`"0"` is true after all, while `0` is not) and do a !! in the check. 

I'll put up a corresponding PR for your review.
@majapw
Copy link
Contributor Author

majapw commented Oct 23, 2020

An alternative would be to expose a script that allows for the end-user to manually install the browser binaries whenever they want. I believe Cypress has this option.

@pavelfeldman
Copy link
Member

Btw, for installing the browsers, you could also run npx playwright-cli

@majapw
Copy link
Contributor Author

majapw commented Oct 23, 2020

Ah! That is very helpful! Looking at the playwright-cli code, it seems like it still goes through the same installer code that has the env variable check (https://github.com/microsoft/playwright-cli/blob/master/src/cli.ts#L128). Would leveraging npx ignore my npmrc config?

Also def seems like a good addition to the installation docs! Happy to add some language there about playwright-cli

@pavelfeldman
Copy link
Member

Would leveraging npx ignore my npmrc config

I am actually not sure, but you can always run it in a separate folder!

Also def seems like a good addition to the installation docs! Happy to add some language there about playwright-cli

That would be awesome! @arjunattam FYI

@pavelfeldman
Copy link
Member

pavelfeldman commented Oct 23, 2020

This is now fixed! Thank you @majapw!

@majapw
Copy link
Contributor Author

majapw commented Oct 23, 2020

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