Skip to content

Conversation

@laurentpellegrino
Copy link
Contributor

This PR generalizes what was originally proposed in #262.

Here is a summary of the spec shared by laurenzlong:

The approved API is to add a field to CallableContext called "rawRequest", which would be the type "express.Request", and it would be the entire HTTP request. Let me know if you have any questions or need any help.

Copy link
Contributor

@laurenzlong laurenzlong left a comment

Choose a reason for hiding this comment

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

Thanks so much! Just some minor comments on the test.

});
});

it('should handle raw request', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

rephrase to: "should expose raw request"

httpRequest: mockRequest,
expectedData: null,
callableFunction: (data, context) => {
expect(context.auth).to.be.undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

This line can be removed, distracts from seeing what the point of the test is.

expectedData: null,
callableFunction: (data, context) => {
expect(context.auth).to.be.undefined;
expect(context.instanceIdToken).to.be.undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, this line can be removed.

@laurentpellegrino laurentpellegrino force-pushed the https-callable-raw-request branch from e0d8d55 to e03ccc2 Compare June 20, 2018 07:57
@laurentpellegrino
Copy link
Contributor Author

Thanks for the review. Your suggestions have been applied.

@laurenzlong
Copy link
Contributor

Looks great! Thanks again for taking the time to do this!

@laurenzlong laurenzlong merged commit 33435b5 into firebase:master Jun 20, 2018
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