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

CLI: Fix spawning child processes on windows #19019

Merged
merged 2 commits into from Sep 7, 2022

Conversation

Delagen
Copy link
Contributor

@Delagen Delagen commented Aug 25, 2022

Issue: #19020

node.js modules .bin commands does not appear available as executable, so invoke these commands as shell commands

What I did

Create angular-cli project
Add storybook 7 version

Cause NOENT error when running compodoc via npx

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

… as executable, so invoke these commands as shell commands
@Delagen
Copy link
Contributor Author

Delagen commented Aug 25, 2022

#19020

@kylegach kylegach requested review from a team and matheo August 29, 2022 15:03
@kylegach
Copy link
Contributor

@storybookjs/angular — Mind taking a look at this one? 🙏

@Marklb
Copy link
Member

Marklb commented Aug 29, 2022

This looks like the same as what I did to fix this on Windows and Stackblitz in #16728, but I missed #18140 that removed my change. So, this change should fix Windows again, but I am not sure if it will reintroduce another issue.

I don't have a macOS to try and reproduce #18140, but I can try to reproduce it on Windows and WSL later.

@Delagen
Copy link
Contributor Author

Delagen commented Aug 29, 2022

@Marklb it looks like you fix only angular compodoc calls, but library has many calls via npx, that can also fail in windows. I think this PR wouldn't create any new issues.

@Marklb
Copy link
Member

Marklb commented Aug 29, 2022

@Delagen that is correct that this applies the change to more places, I just meant the fix itself being the same. This change seems like a higher priority than #18140, since it seems to be avoided by not putting the project or Compodoc output path in a directory where the path contains a space.

If I understand what #18140 was for, I think it is fixing a problem where a file in the workspace itself contains a space. So the fix to just alter the path to be relative in #17334 would not fix it. That issue may not be something necessary for us to fix anyway. It references compodoc/compodoc#1222, so it may be a Compodoc issue. I am not sure if shell: true causes whatever shell was getting spawned on macOS to not work anymore. I tried running Compodoc in Powershell and Command Prompt directly, where the destination path contained a space, and I couldn't figure out a way escape or wrap the path string in a way that Compodoc would accept it, so I don't know if there is anything else we could do in Storybook to avoid that error.

@Delagen
Copy link
Contributor Author

Delagen commented Aug 29, 2022

@Marklb No issues with latest compodoc on Windows 11. At least in my project does not occur any.

Yes. If project path contains spaces error occurs, but I think path must be quoted manually. And i think it a less issue than not work at all.

I made argument as

const finalCompodocArgs = [
            'compodoc',
            // Default options
            ...(hasTsConfigArg(compodocArgs) ? [] : ['-p', tsConfigPath]),
            '-d',
            JSON.stringify(`${context.workspaceRoot}`),
            ...compodocArgs,
        ];

So it works, but I am not sure on another platforms

@shilman shilman changed the title fix(windows): fix windows compatibility CLI: Fix spawning child processes on windows Sep 7, 2022
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much @Delagen @Marklb ! I agree that it's better to get windows working than to handle the corner case of spaces in the filename. Those users will just need to quote or escape their paths. Merging!

@shilman shilman merged commit 8e256f8 into storybookjs:next Sep 7, 2022
@Delagen Delagen deleted the fix-windows-run branch September 12, 2022 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants