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

Sandboxes: Fix scripts when pre-existing NODE_OPTIONS contains whitespaces #22677

Merged
merged 2 commits into from
Sep 12, 2023

Conversation

sookmax
Copy link
Contributor

@sookmax sookmax commented May 22, 2023

What I did

Hi guys. I've noticed the quotation marks in storybook and build-storybook npm scripts in a generated sandbox's package.json (e.g., sandbox/react-vite-default-ts/package.json) are incompatible with situations when pre-existing NODE_OPTIONS contains a string value with whitespaces (e.g., '--require "/Users/sook/Library/Application Support/Code/User/workspaceStorage/c9671de157069c282309baa6cfbc061f/ms-vscode.js-debug/bootloader.js" ' notice there's a whitespace in Application Support).

Currently, the generated storybook script uses double quotes to wrap the final value of NODE_OPTIONS which will conflict with pre-existing double quotes that were escaping whitespaces (if any).

This can be prevented by simply switching the outermost double quotes to single quotes (i.e., NODE_OPTIONS="${nodeOptionsString}" to NODE_OPTIONS='${nodeOptionsString}').

And.. that's all I did.

How to test

  1. run mkdir -p "/tmp/my space" && touch "/tmp/my space/temp.js" && NODE_OPTIONS='--require "/tmp/my space/temp.js"' yarn start ; rm -r "/tmp/my space"
  • this should fail to start the dev server
  1. run mkdir -p "/tmp/my space" && touch "/tmp/my space/temp.js" && yarn task --task dev --template react-vite/default-ts ; rm -r "/tmp/my space" and choose the first option Run the sandbox in development mode (dev)
  • this should fail with the message saying command not found: space/temp.js --preserve-symlinks --preserve-symlinks-main

After applying the suggested fix, running above again will successfully boot up the dev server.

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
    • I ran yarn test and yarn task --task e2e-tests --template=react-vite/default-ts --start-from=auto
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@JReinhold
Copy link
Contributor

what happens if the pre-existing NODE_OPTIONS uses single-quotes instead of double quotes? Or maybe that is not valid?

Ie. if NODE_OPTIONS="--require '/Users/sook/Library/Application Support/Code/User/workspaceStorage/c9671de157069c282309baa6cfbc061f/ms-vscode.js-debug/bootloader.js'"

@sookmax
Copy link
Contributor Author

sookmax commented May 22, 2023

@JReinhold It seems like the recommended way to escape whitespaces is to use single quotes to wrap the outermost and escape whitespaces using double quotes inside, according to Node.js docs.

P.S. Wow the hash tag of the link is very broken. I suggest searching (NODE_OPTIONS=options...) after clicking the above link.

@JReinhold JReinhold self-assigned this May 22, 2023
@yannbf yannbf added the build Internal-facing build tooling & test updates label May 25, 2023
@JReinhold JReinhold changed the title fix: sandbox package.json when pre-existing NODE_OPTIONS value contains whitespaces Sandboxes: Fix scripts when pre-existing NODE_OPTIONS contains whitespaces Aug 29, 2023
@JReinhold JReinhold merged commit bad34e1 into storybookjs:next Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Internal-facing build tooling & test updates ci:normal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants