feat: update node_npm actions and workflow to support building in source directory #462
Conversation
| if self.install_links: | ||
| command.append("--install-links") | ||
|
|
||
| self.subprocess_npm.run(command, cwd=self.install_dir) |
There was a problem hiding this comment.
I've realized the CI action maybe should've just been a subclass of the normal install action, and in general I'm not sure why we don't use the same flags. But it might be too late to change now.
| subprocess_npm=subprocess_npm, | ||
| osutils=osutils, | ||
| build_options=self.options, | ||
| install_links=self.build_dir == self.source_dir, |
There was a problem hiding this comment.
We want to use install-links so npm installs local dependencies without any symbolic links and without dev dependencies or temporary files, but instead just like any other dependency.
| self.subprocess_npm.run(["ci"], cwd=self.install_dir) | ||
| command = ["ci"] | ||
| if self.install_links: | ||
| command.append("--install-links") |
There was a problem hiding this comment.
something to keep in mind, is that we are appending in certain places and doing a + operation in other places, let's default to one.
There was a problem hiding this comment.
I don't think there's a problem with using both 🤔 . Appending to add one element, using "+" to concatenate two lists.
There was a problem hiding this comment.
I don't have any strong opinion here, but we can also use .extend() to concatenate two lists if we feel that's cleaner moving forward, though I think += is usually descriptive enough.
There was a problem hiding this comment.
I understand that its not a technical problem, its about cohesion.
|
Thanks for the reviews! |
Issue #, if available:
Description of changes:
Updates node actions and workflow to support building in source directory. Seems like a big PR but most is tests and changing comments to numpy docstring format. Tests will fail until this other PR is merged: #461
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.