-
Notifications
You must be signed in to change notification settings - Fork 81
Include node on typedoc command #297
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
base: master
Are you sure you want to change the base?
Conversation
|
I guess there are more issues with Windows apart from running typedoc. What I'm seeing now is:
|
| dir = Path(__file__).parent.resolve() / "js" | ||
| command.add("--tsconfig", str(dir / "tsconfig.json")) | ||
| command.add("--import", str(dir / "registerImportHook.mjs")) | ||
| command.add("--import", "file:///" + str(dir / "registerImportHook.mjs")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like there must be a cleaner way to add a protocol to a path.
| source_paths = abs_source_paths | ||
| if os.name == "nt": | ||
| source_paths = map( | ||
| lambda path: str(posixpath.join(*str(path).split(os.sep))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate this, my first attempt was path.split(os.sep) but that errors out because WindowsPath doesn't have a split apparently.
Would love suggestions on how to make this better, the only thing that's important here is that these source paths use forward slashes.
|
It works! I'd appreciate help cleaning up the changes and running all checks locally but these changes were enough to get it working on my Windows machine. |
|
Can you add a windows CI job? |
|
And it looks like CI is broken so I'll have to fix that first... |
|
With some pointers I can look into why CI is broken, I just didn't know where to start since the error is on a file that wasn't changed. |
Closes #285
I considered updating
search_node_modulesto return a command that may or may not include"node"but since including it fixes the Windows issue without breaking other platforms I went with this smaller change instead. This is also consistent with thejsdocinvocation where"node"is always part of the command.On my windows machine the version call succeeds now but the project I'm testing it on is throwing an unrelated error about ESM URL schemes. Because of that I still haven't gotten a successful run after my changes, working on it now.
nox is also causing errors when I run it locally so after I get a successful run I'll try and check the tests locally with WSL.