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

fix(plugin-pnp): fix NODE_OPTIONS parsing in setupScriptEnvironment #6439

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

paul-soporan
Copy link
Member

What's the problem this PR addresses?

setupScriptEnvironment uses a simple regexp to detect whether the hook is injected inside NODE_OPTIONS in order to remove it, but that check is flawed:

  • Doesn't support --require's shorthand: -r
  • Doesn't support double quoted paths (and in turn, doesn't support escaping via backslash).
  • Doesn't support bound arguments (--require= / --experimental-loader=)
  • Wrongly detects parts of other arguments (--report-dir "/path/to/--require .pnp.cjs")

Closes #3232 (supersedes).

How did you fix it?

Implemented a basic regexp argument parser that can handle arguments composed of both plain and double quoted segments.
Double-quoted segments support escaping via backslash, just like Node's parser allows.
Single-quoted segments aren't allowed by Node.

Note that this isn't about the classic meaning of "parsing CLI arguments" (which util.parseArgs and clipanion do), but about parsing a string into the argv array for further processing.

I added tests for everything.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

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.

1 participant