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

fix: allow source folder copy task to dereference symlinks #424

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

benmarch
Copy link

Issue #, if available:

Description of changes:

I have my skill source code set up as a package in a monorepo, and the symlinked NPM modules (the other packages) are not copied into the build folder. This change instructs fs-extra to also copy the symlinked modules.

I wasn't sure if it was worth writing new tests for this change or updating the existing ones. I could also make this a config option for CodeBuilder instead of hardcoding it to true (and adding a cli option to set it). Any input or advice would be appreciated.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@Shreyas-vgr
Copy link
Contributor

Hi @benmarch, thanks for the PR
Yes adding it as a config option is better than hardcoding and also feel free to add new test cases for the changes :)

@benmarch
Copy link
Author

benmarch commented Feb 5, 2022

@Shreyas-vgr apologies for the delay. I've added two config options: dereference-symlinks and no-npm-install. I'm not sure the latter makes sense as a config option because it only affects an Node/NPM-based skill, but it was necessary to get my skill working.

After these changes, I realized that I could use a custom hook to accomplish the same thing, but I think there's something lost by using a hook instead of the built-in support for Node skills.

Let me know what you think, thanks!

@CamdenFoucht CamdenFoucht requested review from CamdenFoucht, doiron and tydonelson and removed request for Shreyas-vgr August 4, 2022 20:25
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.

3 participants