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

Angular: Fix runCompodoc for Windows, local Compodoc, and user specified tsconfig #16728

Merged
merged 9 commits into from
Jan 3, 2022

Conversation

Marklb
Copy link
Member

@Marklb Marklb commented Nov 19, 2021

What I did

I got an error when the builder tried to call compodoc with spawn on Windows. In other apps I have fixed spawning child processes on Windows, by specifying the shell, and that seemed to fix my issue here also.
Based on the docs https://nodejs.org/api/child_process.html#child_process_child_process_spawn_command_args_options, I should be able to just pass true, instead of a specific shell. So, I will do some testing with that tonight and maybe be able to remove the OS check.

I don't install compodoc globally, and wouldn't expect the global version to be used over the local version, so I set the cwd of the spawned shell to the workspace root and spawned npx instead of compodoc.

I have run into situations where a compodoc bug caused it to fail when parsing a file that wasn't even necessary for my stories. So, I started using a separate tsconfig to exclude those files. So, I added a check for "-p" in "compodocArgs" and only add the tsconfig from the angular build config if "-p" isn't provided.

I don't know enough about the StackBlitz WebContainers to know if a browser being on Windows would affect that environment, but I wouldn't think so. This fix does fix my StackBlitz projects using the builders though. So, If a browser being on Windows has no effect on the WebContainers then this fixes Windows and StackBlitz WebContainers.

How to test

  • Is this testable with Jest or Chromatic screenshots? The code functionality, yes. Testing that it works on a Windows OS, would depend on if the tests are run on a Windows OS. I am going to hopefully get this working without the OS specific code, but I still don't understand why I only get this problem on Windows, so I can't guarantee the test will stay valid without it running on Windows.
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

@nx-cloud
Copy link

nx-cloud bot commented Nov 19, 2021

☁️ Nx Cloud Report

CI ran the following commands for commit 14718f3. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@MichaelArestad
Copy link
Contributor

@storybookjs/angular Could one of y'all take a look at this, please?

@ThibaudAV
Copy link
Contributor

I will see for test on mac ;)
but I don't have windows for dev (only for games 😁 )

@Marklb
Copy link
Member Author

Marklb commented Nov 23, 2021

I switched the implementation to set shell to true, instead of specifying a specific shell. So, it is less OS specific now, but still seems to work on Windows.

@Marklb
Copy link
Member Author

Marklb commented Dec 3, 2021

This same error happens on Stackblitz WebContainers, but I haven't tried this fix on there yet. I may be able to use a local Verdaccio repository with this branch published or maybe a script to patch the file in node_modules. I can try to test it on there later.

@shilman
Copy link
Member

shilman commented Dec 3, 2021

@Marklb can you please update from next and resolve the merge conflicts here? thanks! 🙏

@Marklb
Copy link
Member Author

Marklb commented Dec 4, 2021

Fixed tests. Also, checked that it fixed builders running Compodoc in a StackBlitz WebContainer. I assume those containers are totally sandboxed to where they would not be inheriting this behavior from the OS, but I am not positive.

@Marklb
Copy link
Member Author

Marklb commented Jan 1, 2022

Anyone able to take a look at this? On Windows and Stackblitz, at least, I have to disable Compodoc being run by the builder and add the script back to run it before the builder.

Should I force push a commit that doesn't have the app/angular/src/server/framework-preset-angular-cli.ts and lib/core-server/src/build-dev.ts changes? The linter run by the pre-commit hook does those two changes.

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.

LGTM! Thanks @Marklb 🙏

@shilman shilman merged commit 578082b into storybookjs:next Jan 3, 2022
@Darkmift
Copy link

Darkmift commented Jan 18, 2022

Hi all,

I came across this issue as well today and yesterday,

node:internal/errors:464
    ErrorCaptureStackTrace(err);
    ^

Error: spawn UNKNOWN
    at ChildProcess.spawn (node:internal/child_process:412:11)
    at Object.spawn (node:child_process:698:9)
    at module.exports (C:\...\node_modules\better-opn\node_modules\open\index.js:175:34) {
  errno: -4094,
  code: 'UNKNOWN',
  syscall: 'spawn'
}

Ill note I tried running the project in several Pc's and only some had this issue.

I would appreciate any help on solving this.

**Edit

seems Powershell is restricted on the PC having issues.
so this is not to do with the npm at all.

Apologies.

P.s.
I tried setting the defaut shell for npm but this had no effect...if anyone knows which config to edit and how so the command will path to another shell I would really appreciate the help.

Edit:

I tried to manually tweak the code but this failed,I lack the knowledge to make this work right

987987987

@shilman shilman added patch:yes Bugfix & documentation PR that need to be picked to main branch patch:done Patch/release PRs already cherry-picked to main/release branch and removed patch:done Patch/release PRs already cherry-picked to main/release branch labels Jan 26, 2022
shilman added a commit that referenced this pull request Jan 28, 2022
Angular: Fix runCompodoc for Windows, local Compodoc, and user specified tsconfig
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jan 29, 2022
@sir-captainmorgan21
Copy link

@shilman is there a builder out there that runs compodoc as a part of the storybook build? Any docs on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
angular bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants