Skip to content

Conversation

@okko
Copy link

@okko okko commented Aug 29, 2022

Fixes #21630.

Stack filenames contain file:// prefix in ES module files. This removes the prefix for Node compatibility.


All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Aug 29, 2022

@github-actions github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p2 labels Aug 29, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team August 29, 2022 12:25
@okko okko changed the title Fix default entry finding when Node returns paths starting with file:// fix(aws-lambda-nodejs): Default entry finding when Node returns paths starting with file:// Aug 29, 2022
@okko
Copy link
Author

okko commented Aug 29, 2022

@corymhall Please advice if I should change findEntry to be exported so that I can add tests for this case. I'm not sure how to create a test scenario using only the currently exported NodejsFunction.

@corymhall
Copy link
Contributor

Please advice if I should change findEntry to be exported so that I can add tests for this case. I'm not sure how to create a test scenario using only the currently exported NodejsFunction.

I think we can add a test in function.test.ts. It seems like all that is needed to reproduce the issue is to have a package.json with "type": "module". If that is the case we could just add new test directory with a package.json and handler file.

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see an integration test for this. It's not part of our PRLinter just yet (change coming soon) but we are now requiring integ tests on fixes.

@okko
Copy link
Author

okko commented Aug 31, 2022

An integration test would require the test tooling to compile the test case project to ESM and then synthesize that. I believe that would require a separate package.json, tsconfig.json, jest config, cdk.json that executes ts-node in esm mode, and hooking that into the way the tests run. I'm not familiar enough with the aws-cdk development tooling to do all of that, and I think that shouldn't be in the scope of this fix. I have provided all the needed files at https://github.com/okko/aws-cdk-esm-file-urls-reproduce for reference.

At the moment this blocks us from using NodejsFunction with a project compiling to ESM.

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review August 31, 2022 07:34

Pull request has been modified.

@corymhall
Copy link
Contributor

An integration test would require the test tooling to compile the test case project to ESM and then synthesize that. I believe that would require a separate package.json, tsconfig.json, jest config, cdk.json that executes ts-node in esm mode, and hooking that into the way the tests run. I'm not familiar enough with the aws-cdk development tooling to do all of that, and I think that shouldn't be in the scope of this fix. I have provided all the needed files at okko/aws-cdk-esm-file-urls-reproduce for reference.

We should update the integ.esm.ts integration test as part of this PR. Right now it is just using the test/integ-handlers/esm.ts as the entry, but we should be able to add a subfolder test/integ-handlers/esm/esm.ts and add a package.json

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting this back into changes requested in regard to updating the test.

@koshic
Copy link

koshic commented Sep 16, 2022

@okko any updates? Tricks with 'entry: new URL(import.meta.url.replace(/(.*).(.+)/, $1.${id}.$2)).pathname' are so ugly.

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review October 6, 2022 13:35

Pull request has been modified.

Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 21fa23e
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@TheRealAmazonKendra
Copy link
Contributor

This PR has been deemed to be abandoned, and will be closed. Please create a new PR for these changes if you think this decision has been made in error.

mergify bot pushed a commit that referenced this pull request Oct 22, 2025
### Issue # (if applicable)

Closes #21630.

### Reason for this change

In the ESM module system, callsites returns filenames prefixed with 'file://'. This is not compatible with the NodeJS file utility functions such as fs.existsSync().

### Description of changes

Remove 'file://' prefix.

### Describe any new or updated permissions being added

No new IAM permissions are added.

### Description of how you validated changes

Unit tests. Integration test cannot be added as the integration test system uses CommonJS, it will be impossible to replicate the error which only happens under an ESM system. 

### Credits

This is mostly inspired by @okko for [his PR](#21802). He found the root cause and fix for this issue, but it didn't get merged due to a lack of integration tests.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

aws-lambda-nodejs: Default entry finding fails when defining file starts with "file://"

5 participants