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. |
mattsains
changed the title
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
@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.
| 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.
@eladb can you have a look?
There was a problem hiding this comment.
I will try this out, thanks for the suggestion!
There was a problem hiding this comment.
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.
Yes this is how I would do it
There was a problem hiding this comment.
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.
| 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.
| 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