Skip to content
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 headers; Small improvements; Add integration tests; #20

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

D-Andreev
Copy link
Contributor

@D-Andreev D-Andreev commented Dec 25, 2021

The key should be headers, otherwise there is an exception Invalid lambda response received: Invalid API Gateway Response Keys: {'header'}.

@buildbreakdo
Copy link
Owner

buildbreakdo commented Dec 28, 2021

Thanks for the fix!

New build/index.js is ~8x the size of the previous build/index.js file for a 1 character change, something must be different beyond the one line change. Maybe new webpack version was used to produce the build? Build folder should have never been committed (build is in .gitignore), mistake many years ago.

Happy to merge this with build/index.js deleted.

@D-Andreev
Copy link
Contributor Author

D-Andreev commented Dec 28, 2021

Maybe new webpack version was used to produce the build?

Probably it's because of the version. Still quite the difference. I have version 4.46.0 of webpack installed.
But yeah, build should be ignored. Removing it in this PR 😃

const fetch = require('node-fetch');

let samProcess;
const port = 5000;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know this port is used only for local development, but does it make sense to come from an environment variable, from .env file or something?

const port = 5000;
let lambdaUrl = `http://127.0.0.1:${port}/`;
function localApiStarted(processOutput) {
// Example output when local function started - "Running on http://127.0.0.1:5000/"
Copy link
Contributor Author

@D-Andreev D-Andreev Dec 28, 2021

Choose a reason for hiding this comment

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

beforeAll hook starts the function locally and waits for output that sam has started, so the tests can begin. Probably not ideal way of doing it, I'm open to suggestions how to do this better :)

"start": "sam local start-api --port 5000",
"test:integration": "npm run build && jest --verbose=false --config ./jest.config.integration.js",
"test:watch:integration": "npm run build && jest --watch --verbose=false --config ./jest.config.integration.js",
"start": "sam local start-api --port 5000 2>&1 | tr \"\\r\" \"\\n\"",
Copy link
Contributor Author

@D-Andreev D-Andreev Dec 28, 2021

Choose a reason for hiding this comment

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

Added this part 2>&1 | tr \"\\r\" \"\\n\" because the console.logs from inside the function were not shown. A workaround, but couldn't find anything better - #1359

}

return callback(null, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When function is async the callback is not needed, you can just return result.

@D-Andreev
Copy link
Contributor Author

Hey @buildbreakdo,

I made some small improvements and added an integration test. Can you take a look please?

@D-Andreev D-Andreev changed the title Fix headers Fix headers; Small improvements; Add integration tests; Dec 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants