-
-
Notifications
You must be signed in to change notification settings - Fork 460
Add a binary to allow use without copying example #688
Conversation
This makes running builds simpler for non-serverless projects that use this library.
Codecov Report
@@ Coverage Diff @@
## master #688 +/- ##
==========================================
- Coverage 80.18% 79.72% -0.47%
==========================================
Files 54 55 +1
Lines 1731 1741 +10
Branches 366 368 +2
==========================================
Hits 1388 1388
- Misses 285 295 +10
Partials 58 58
Continue to review full report at Codecov.
|
Thanks, ran e2e tests workflow here: https://github.com/serverless-nextjs/serverless-next.js/actions/runs/311606442 |
|
||
async function main (args: string[]) { | ||
if (args.length > 1) { | ||
console.error("Usage: build-lambda-at-edge [ NEXT_APP_DIR ]") |
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.
Please run prettier to reformat this whole file, it seems it is not formatted as it has inconsistent semicolons at end of line (it should be part of git hook). Also if possible a simple unit test would be good too. Thanks!
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.
Hi @dphang
Thanks for the feedback. I've pushed a commit with the requested formatting using prettier. I'm not sure how to meaningfully unit test this functionality - perhaps mocking out and injecting the Builder, but it seems this would just be a test of the implementation rather than the behaviour. Let me know if you have a suggestion and I'll look at implementing it.
Thanks
Tom
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.
Yeah, I guess just ensure this script reasonably works as expected, mocking should be fine but if you don't think it's too useful, I'm fine as well since this is not part of the core lambda-at-edge source code.
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 started to look at this but it wasn't immediately obvious how to run the unit tests for the lambda-at-edge module. If you're happy to go with it as is then that would be fine by me, or I'm happy to write a test if you can give me some guidance on how to get the tests running. Thanks
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.
We can merge it for now, you can check the CONTRIBUTING.md for how to run tests, we use yarn test lambda-at-edge
which will run the tests there.
Thanks.
This makes running builds simpler for non-serverless projects that use this library.