fix(lambda-nodejs): esbuild define parameters are incorrectly encoded #14065
Conversation
|
|
|
There's still invalid edge cases. |
|
|
skyrpex
force-pushed
the
skyrpex/fix-esbuild-define-parameters
branch
from
fb537d9 to
38bfb8c
Compare
|
Alright, in order to fix the esbuild define instructions for the CLI, I had to JSON.stringify all the values. Now they work as expected. Added a snapshot test that calls the esbuild CLI to ensure that this doesn't break again. |
| '--log-level=silent --keep-names --tsconfig=/asset-input/lib/custom-tsconfig.ts', | ||
| '--metafile=/asset-output/index.meta.json --banner=\'/* comments */\' --footer=\'/* comments */\'', | ||
| ].join(' '), | ||
| ], | ||
| }), | ||
| }); | ||
|
|
||
| // Make sure that the define instructions are working as expected with the esbuild CLI |
There was a problem hiding this comment.
Do you think we really need to run esbuild here in the test? Isn't it enough to ensure that the right command line args are passed?
There was a problem hiding this comment.
Well, I got it right this time and reverse-engineered the define parameters' expectation (it's a hell plenty of escape slashes). It should be stable now and the test is not exactly necessary, but it's proof that it works now.
Let me know if you want me to remove it 👍🏻
There was a problem hiding this comment.
I like that this is doing an end to end test
Co-authored-by: Jonathan Goldwasser <jogold@users.noreply.github.com>
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
eladb
changed the title
|
@jogold waiting for your approval and I'll remove the do-not-merge label |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Fixes #13842