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

pnpm ignoring detect_chromedriver_version flag in .npmrc #4927

Closed
sbonasu opened this issue Jun 24, 2022 · 15 comments
Closed

pnpm ignoring detect_chromedriver_version flag in .npmrc #4927

sbonasu opened this issue Jun 24, 2022 · 15 comments

Comments

@sbonasu
Copy link

sbonasu commented Jun 24, 2022

pnpm version: 7.3.0

Code to reproduce the issue:

https://www.npmjs.com/package/chromedriver

.npmrc file contents
ignore-scripts = true
detect_chromedriver_version=true

Expected behavior:

browser version is 95.0.4638.54

When I check chromedriver version in node_modules, its not matching chrome browser version

node_modules/.pnpm/node_modules/.bin/chromedriver --version
ChromeDriver 101.0.4951.41 (93c720db8323b3ec10d056025ab95c23a31997c9-refs/branch-heads/4951@{#904})

Actual behavior:

chromedriver version should match browser version.

If the current implementation doesn't support this setup. Is there a way to pass DETECT_CHROMEDRIVER_VERSION=true environment variable with pnpm install or use .pnpmfile.cjs?

I have tried doing this with .pnpmfile.cjs afterAllResolved hook but it doesn't run when Lockfile is up-to-date. Deleting lock file is not an option since it introduces version mismatches(importing from yarn.lock).

function afterAllResolved(lockfile, context) {
  context.log('Installing chromedriver');

  let chromeDriverPath = path.join(__dirname, 'node_modules/.pnpm/node_modules/chromedriver');

  let chromeDriverInstall = spawn(
    'npm',
    ['run', 'install'],
    { cwd: chromeDriverPath },
    { env: { ...process.env, DETECT_CHROMEDRIVER_VERSION: 'true' } },
  );

  chromeDriverInstall.on('error', (err) => {
    context.log('failed to install chromedriver: ' + err);
  });

  return lockfile;
}

Additional information:

  • node -v prints: v16.15.1
  • Windows, macOS, or Linux?: macOS and Linux
@zkochan
Copy link
Member

zkochan commented Jun 27, 2022

I did not know that npm puts setting from .npmrc to env variables when running lifecycle scripts. Does Yarn implement it the same way? Obviously, if it works with npm, we should support it in some way

@sbonasu
Copy link
Author

sbonasu commented Jun 28, 2022

I think yarn@1 implements it.

With yarn@1, we use yarn-path setting to execute bash script that intercepts yarn commands. It does yarn install and then runs post install script that installs chromedriver with env DETECT_CHROMEDRIVER_VERSION

@sbonasu
Copy link
Author

sbonasu commented Jun 29, 2022

@zkochan do you think its possible to add a feature to define settings in .npmrc or modify custom hook afterAllResolved to run scripts(not restrict to just lockfile mutation)

@zkochan
Copy link
Member

zkochan commented Jun 29, 2022

I don't think .npmrc is a good idea. But I think having something like a separate .env file would be fine. Actually @shellscape had this idea.

@shellscape
Copy link
Contributor

What's funny is that I'm on a two week RV trip with my family and doing a lot of driving - which means lots of time to think. I've been thinking about .env support quite a lot. @zkochan when I get back after July 9th I'll probably pick that up.

@zkochan
Copy link
Member

zkochan commented Jun 29, 2022

But if I read .env correctly, it looks like it should not be committed to the repo.

In this case, I believe we want this in the repo.

I think we can add a new field to package.json with envs. Something like:

{
  "pnpm": {
    "envVariables": {
      "detect_chromedriver_version": "true"
    }
  }
}

@sbonasu
Copy link
Author

sbonasu commented Jun 30, 2022

But if I read .env correctly, it looks like it should not be committed to the repo.

In this case, I believe we want this in the repo.

I think we can add a new field to package.json with envs. Something like:

{
  "pnpm": {
    "envVariables": {
      "detect_chromedriver_version": "true"
    }
  }
}

I like envVariables idea

@zkochan
Copy link
Member

zkochan commented Jul 4, 2022

cc @pnpm/collaborators let me know what you think about this feature.

@BlackHole1
Copy link
Member

But if I read .env correctly, it looks like it should not be committed to the repo.

In this case, I believe we want this in the repo.

I think we can add a new field to package.json with envs. Something like:

{
  "pnpm": {
    "envVariables": {
      "detect_chromedriver_version": "true"
    }
  }
}

I Approve of this. But at the same time, we should be consistent with the behavior of npm. And npm will auto add prefix: npm_config_. Like: npm_config_${key.toLowerCase()}. See:

  1. https://github.com/giggio/node-chromedriver/blob/8c122d919ceb0a918bc7ca7fcf2f0f4ab7a89281/install.js#L35
  2. https://github.com/npm/cli/blob/ef8d2edd7da993f4086c85089952cd45834ac78b/docs/content/commands/npm.md#configuration
  3. https://github.com/npm/config/blob/6440fba6e04b1f87e57b4c2ccc5ea84d8a69b823/lib/set-envs.js#L9-L13

@sbonasu
Copy link
Author

sbonasu commented Jul 5, 2022

good catch @BlackHole1. Thank you all for looking in to this

@gluxon
Copy link
Member

gluxon commented Jul 6, 2022

+1 to @BlackHole1's comment. @sbonasu The process.env.npm_config_detect_chromedriver_version environment variable should be visible to the node-chromedriver postinstall script already. Is that the case?

@sbonasu
Copy link
Author

sbonasu commented Jul 6, 2022

@gluxon no, postinstall is not installing correct chromedriver version matching chrome version. Its installing the version specified in package.json

@zkochan
Copy link
Member

zkochan commented Jul 9, 2022

I've checked, npm_config_ is set correctly. The same way as with npm.

Maybe you have this package cached and the cache was written with a different setting.

Try to add side-effects-cache=false to your .npmrc, remove node_modules and reinstall.

@sbonasu
Copy link
Author

sbonasu commented Jul 11, 2022

side-effects-cache=false seems to have done the trick. Thank you

@zkochan
Copy link
Member

zkochan commented Jul 16, 2022

ok, this is solved. I created a new issue to try to fix this #5002

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants