-
Notifications
You must be signed in to change notification settings - Fork 822
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
Custom resource build should use the build
npm script specified in its package.json
#11889
Comments
…rather than invoking tsc directly from the node_modules/.bin directory
Note for fix: ensure all other relative paths looking for |
…rather than invoking tsc directly from the node_modules/.bin directory
@josefaidt we are running into the same issue with
The issue is the same "TypeScript executable not found." error (though that message isn't properly being exposed in the CLI output). The only workaround I've found to getting this working is to disable hoisting in "installConfig": {
"hoistingLimits": "workspaces"
} We are doing the exact same thing as @acusti (#11851 (comment)): Use hoisting to prevent It seems that the design of overrides is less-than-desirable in that the The root cause of the problem is identical to what is being fixed in #11854. The Amplify CLI should not directly rely on I have created #13858 to fix the issue in the exact same way. Would love to see these PRs merged & released so we don't have to use custom |
Updated the TypeScript compilation of overrides so that it doesn't require `node_modules/.bin/tsc`. Instead, it simply relies on the `build` script to execute `tsc`. This is more flexible and can support alternative setups w/ hoisting (e.g. via Yarn workspaces). This is an override corollary fix to #11854, which is for custom resources. #11889
* fix(amplify-cli-core): use build script properly for overrides Updated the TypeScript compilation of overrides so that it doesn't require `node_modules/.bin/tsc`. Instead, it simply relies on the `build` script to execute `tsc`. This is more flexible and can support alternative setups w/ hoisting (e.g. via Yarn workspaces). This is an override corollary fix to #11854, which is for custom resources. This is an improvement over my previous PR in #13858 in that it works with any package manager by ensuring the `--project` and `tsconfig.json` files are passed through to the `tsc` script. The previous implementation didn't work with `npm` because it doesn't pass through additional args like `yarn` does. The fix was easy: simply separate the build run with `--` so that the remaining args are treated as positional for the `tsc` script. I've confirmed the fix works with both `yarn` and `npm` 💪 #11889 * fix(amplify-cli-core): only include -- for npm Tweaked the implementation slightly so that the `--` arg is only passed for `npm` since it doesn't actually work with `yarn`. #11889 * fix(amplify-cli-core): new packageManner runner Added a new `runner` field to the PackageManager interface. The key difference here is npm's runner is actually `npx`, whereas yarn and pnpm just use the same executable as the runner. Updated both the override and custom-resources to use the runner to run tsc now instead of only looking in `node_modules`. #11889 * fix: update api --------- Co-authored-by: Kamil Sobol <5849952+sobolk@users.noreply.github.com>
Is this feature request related to a new or existing Amplify category?
No response
Is this related to another service?
No response
Describe the feature you'd like to request
instead of invoking
node_modules/.bin/tsc
directly to build custom resources, we should instead use the definedbuild
script in the resource'spackage.json
. This script is automatically added when we add a custom resource withamplify add custom
ref #11851
Describe the solution you'd like
see above
Describe alternatives you've considered
n/a
Additional context
No response
Is this something that you'd be interested in working on?
Would this feature include a breaking change?
The text was updated successfully, but these errors were encountered: