-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(aws-lambda-nodejs): Remove dependency on global parcel installation #6206
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
I'm no longer sure whether this would fix the problem, the spawn command might mean that npx is running on the package using aws-lambda-nodejs rather than aws-lambda-nodejs itself. I'm trying to consume my local version of the CDK to see if it works. Edit: I performed a test where I ran I think the answer to my concern above is that parcel is included as a transitive dependency, so as long as you're depending on @aws-cdk/aws-lambda-nodejs, parcel will be included as well, and npx will be able to find it. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
@mattsains what do you think of the suggested solution without npx
? Can you test it with your setup?
@@ -72,7 +73,7 @@ export function build(options: BuildOptions): void { | |||
: [], | |||
].filter(Boolean) as string[]; | |||
|
|||
const parcel = spawnSync('parcel', args); | |||
const parcel = spawnSync('npx', args); |
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.
const parcel = spawnSync('npx', args); | |
const parcelPkgPath = require.resolve('parcel-bundler/package.json'); // eslint-disable-line @typescript-eslint/no-require-imports | |
const parcelDir = path.dirname(parcelPkgPath); | |
const parcelPkg = require(parcelPkgPath); // eslint-disable-line @typescript-eslint/no-require-imports | |
const binPath = path.join(parcelDir, parcelPkg.bin.parcel); | |
const parcel = spawnSync(binPath, args); |
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.
@eladb can you have a look?
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 will try this out, thanks for the suggestion!
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.
that worked for me, and I think it's the right way to go, I'll update this PR
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.
Yes this is how I would do it
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 updated the PR with that suggestion.
@@ -31,7 +31,8 @@ test('NodejsFunction', () => { | |||
// THEN | |||
const { spawnSync } = require('child_process'); // eslint-disable-line @typescript-eslint/no-require-imports | |||
|
|||
expect(spawnSync).toHaveBeenCalledWith('parcel', expect.arrayContaining([ | |||
expect(spawnSync).toHaveBeenCalledWith('npx', expect.arrayContaining([ |
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.
expect(spawnSync).toHaveBeenCalledWith('npx', expect.arrayContaining([ | |
expect(spawnSync).toHaveBeenCalledWith(expect.stringContaining('parcel-bundler'), expect.arrayContaining([ |
@@ -50,7 +51,8 @@ test('NodejsFunction', () => { | |||
])); | |||
|
|||
// Automatically finds .js handler file | |||
expect(spawnSync).toHaveBeenCalledWith('parcel', expect.arrayContaining([ | |||
expect(spawnSync).toHaveBeenCalledWith('npx', expect.arrayContaining([ |
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.
expect(spawnSync).toHaveBeenCalledWith('npx', expect.arrayContaining([ | |
expect(spawnSync).toHaveBeenCalledWith(expect.stringContaining('parcel-bundler'), expect.arrayContaining([ |
…lly installed cdk cli Fixes #6204
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request is now being automatically merged. |
Fixes #6204
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license