fix: prevent browserslist from reading pnpm shim files#2027
fix: prevent browserslist from reading pnpm shim files#2027HuiNeng6 wants to merge 1 commit intoasyncapi:masterfrom
Conversation
Set BROWSERSLIST_ROOT_PATH environment variable during template compilation to prevent browserslist from searching outside the template directory. This fixes an issue where pnpm's shim files were incorrectly parsed as browserslist configuration, causing 'Unknown browser query' errors. Fixes: asyncapi/cli#1781
HuiNeng6
requested review from
asyncapi-bot-eve,
derberg,
jonaslagoni and
magicmatatjahu
as code owners
🦋 Changeset detectedLatest commit: c40721c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
What reviewer looks at during PR reviewThe following are ideal points maintainers look for during review. Reviewing these points yourself beforehand can help streamline the review process and reduce time to merge.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
📝 WalkthroughWalkthroughThis patch fixes a browserslist resolution issue when using pnpm by setting the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/generator/lib/generator.js (3)
399-402: TightenconfigureTemplateJSDoc to include return and error behavior.The function behavior now includes temporary env mutation and restoration around async compilation; document
@returnsand failure behavior to match project JSDoc expectations.As per coding guidelines: "
**/*.{js,ts,jsx,tsx}: Write clear and consistent JSDoc comments for functions, including parameter types, return values, and error conditions".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/generator/lib/generator.js` around lines 399 - 402, Update the JSDoc for async function configureTemplate() to explicitly document its return and error behavior: add an `@returns` {Promise<void>} noting it resolves when template configuration and async compilation complete, describe that it temporarily mutates process.env (or the relevant env object) during compilation and restores the original environment afterwards, and state the error condition/throws behavior (e.g., it will reject/throw if template compilation fails). Reference the configureTemplate method name and mention the temporary env mutation/restoration so the comment matches the implementation.
402-422: Add a regression test for the pnpm shim/browserslist case.This fix is important and env-sensitive; please add a test in the template test suite that exercises compile mode and verifies pnpm-like shim content is not treated as browserslist config.
Based on learnings: "Template features should be tested using the
react-templateinapps/generator/test/test-templatesbefore committing".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/generator/lib/generator.js` around lines 402 - 422, Add a regression test that runs the generator in compile mode (so configureTemplate calls configureReact with TRANSPILED_TEMPLATE_LOCATION) using the react-template test fixture, and assert that a file containing pnpm-like shim content in the templateContentDir is not detected/parsed as a browserslist config when BROWSERSLIST_ROOT_PATH is set to the templateDir; specifically create a test that creates a pnpm-shim-like file in the template, invokes the compile flow that triggers configureTemplate/configureReact, and verifies output/behavior shows the shim was ignored (no browserslist parsing error and expected compiled output exists).
404-419: Process-wide env mutation is race-prone under concurrent generation.This mutates
process.env.BROWSERSLIST_ROOT_PATHglobally during anawait. If two generations run in parallel (different templates), one run can observe the other run’s root path.Possible hardening (serialize the env-scoped compile section)
+let compileEnvLock = Promise.resolve(); class Generator { async configureTemplate() { if (this.compile) { + const previousLock = compileEnvLock; + let releaseLock; + compileEnvLock = new Promise(resolve => { releaseLock = resolve; }); + await previousLock; + const previousRootPath = process.env.BROWSERSLIST_ROOT_PATH; process.env.BROWSERSLIST_ROOT_PATH = this.templateDir; try { await configureReact(this.templateDir, this.templateContentDir, TRANSPILED_TEMPLATE_LOCATION); } finally { if (previousRootPath === undefined) { delete process.env.BROWSERSLIST_ROOT_PATH; } else { process.env.BROWSERSLIST_ROOT_PATH = previousRootPath; } + releaseLock(); } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/generator/lib/generator.js` around lines 404 - 419, The code temporarily mutates process.env.BROWSERSLIST_ROOT_PATH around an await in the call to configureReact, which is race-prone for concurrent generations; to fix it, avoid in-process global env mutation by either (A) running the configureReact step in a separate child process with an overridden env that sets BROWSERSLIST_ROOT_PATH (so each generation gets its own env), or (B) if staying in-process, serialize access by adding a mutex/lock around the section that sets/restores BROWSERSLIST_ROOT_PATH so only one caller at a time performs the set -> await configureReact(...) -> restore sequence; update the call sites using templateDir, templateContentDir, and TRANSPILED_TEMPLATE_LOCATION accordingly and ensure the lock is acquired before setting BROWSERSLIST_ROOT_PATH and released after restoring it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/generator/lib/generator.js`:
- Around line 399-402: Update the JSDoc for async function configureTemplate()
to explicitly document its return and error behavior: add an `@returns`
{Promise<void>} noting it resolves when template configuration and async
compilation complete, describe that it temporarily mutates process.env (or the
relevant env object) during compilation and restores the original environment
afterwards, and state the error condition/throws behavior (e.g., it will
reject/throw if template compilation fails). Reference the configureTemplate
method name and mention the temporary env mutation/restoration so the comment
matches the implementation.
- Around line 402-422: Add a regression test that runs the generator in compile
mode (so configureTemplate calls configureReact with
TRANSPILED_TEMPLATE_LOCATION) using the react-template test fixture, and assert
that a file containing pnpm-like shim content in the templateContentDir is not
detected/parsed as a browserslist config when BROWSERSLIST_ROOT_PATH is set to
the templateDir; specifically create a test that creates a pnpm-shim-like file
in the template, invokes the compile flow that triggers
configureTemplate/configureReact, and verifies output/behavior shows the shim
was ignored (no browserslist parsing error and expected compiled output exists).
- Around line 404-419: The code temporarily mutates
process.env.BROWSERSLIST_ROOT_PATH around an await in the call to
configureReact, which is race-prone for concurrent generations; to fix it, avoid
in-process global env mutation by either (A) running the configureReact step in
a separate child process with an overridden env that sets BROWSERSLIST_ROOT_PATH
(so each generation gets its own env), or (B) if staying in-process, serialize
access by adding a mutex/lock around the section that sets/restores
BROWSERSLIST_ROOT_PATH so only one caller at a time performs the set -> await
configureReact(...) -> restore sequence; update the call sites using
templateDir, templateContentDir, and TRANSPILED_TEMPLATE_LOCATION accordingly
and ensure the lock is acquired before setting BROWSERSLIST_ROOT_PATH and
released after restoring it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 25c633fc-8b80-4ff1-aacd-2d83534feb6f
📒 Files selected for processing (2)
.changeset/fix-browserslist-pnpm.mdapps/generator/lib/generator.js
|
which generator, cli and html-templates versions were you testing this PR with |
3 participants
Description
This PR fixes an issue where using pnpm to install asyncapi/cli globally causes a browserslist error when generating documentation.
Problem
When using pnpm to install asyncapi/cli globally, the generator fails with the error:
This happens because pnpm creates shell script shims for globally installed packages, and browserslist incorrectly parses these shim files as configuration files during template compilation.
Solution
Set the
BROWSERSLIST_ROOT_PATHenvironment variable during template compilation to restrict browserslist's search scope to the template directory only. This prevents browserslist from traversing to pnpm's shim directories and incorrectly parsing shell scripts as config files.Related Issue
Fixes asyncapi/cli#1781
Testing
This fix has been tested with:
Summary by CodeRabbit