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

AMP headers upfront #162

Merged
merged 2 commits into from
Dec 6, 2017
Merged

AMP headers upfront #162

merged 2 commits into from
Dec 6, 2017

Conversation

maxime-michel
Copy link

I now get the headers I added to server.js, but not those in process.js. This isn't my area of expertise but I'm afraid it might be too late to send headers at that point.

Let's do it upfront like in this example: https://github.com/ampproject/amphtml/blob/ee2dea74e954ceaf5c284bfbfb2f81416858fa9a/build-system/test-server.js#L26

Headers make more sense in the same place anyway. Sorry for the back and forth but I don't know how to run this locally, and I've never debugged JS anyway.

@eduardoboucas
Copy link
Owner

Why have you removed the tests?

@maxime-michel
Copy link
Author

Because sadly the relocation of the logic from process.js to server.js means this is not as easily covered. In the scope of the process controller, the mock response will no longer see calls to headers, as this is done upfront. I don't see any other place where the tests could fit, I actually don't see any test for methods in server.js. In the end the logic I'm adding there is pretty minimal, though, isn't it?

@maxime-michel
Copy link
Author

Let's try this and see if it works? I've just given it a new try but I don't see any test class in which headers could be inspected. We can always add tests later if needed.

@eduardoboucas
Copy link
Owner

Sorry, this went off my radar. Let's give this a try, merging now.

@eduardoboucas eduardoboucas merged commit 2d3f7fd into eduardoboucas:dev Dec 6, 2017
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