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

Support CORS headers #271

Closed
wants to merge 5 commits into from
Closed

Support CORS headers #271

wants to merge 5 commits into from

Conversation

MarhiievHE
Copy link
Member

metarhia/impress#1687 (comment)

  • tests and linter show no problems (npm t)
  • code is properly formatted (npm run fmt)
  • description of changes is added in CHANGELOG.md

Copy link
Member

@tshemsedinov tshemsedinov left a comment

Choose a reason for hiding this comment

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

  • It would be better to add HEADERS in Server class to do it once before server port open.
  • We should support old versions of impress and config files, so we can't expect that config.server.cors.origin is present, need a dynamic check and use it only if it exists.

@tshemsedinov tshemsedinov changed the title CORS headers Support CORS headers Oct 29, 2021
@tshemsedinov
Copy link
Member

Moving all HEADERS to lib/server.js is not ok, it's better to hold them in lib/http.js, just patch from server, but exporting/importing HEADERS also not a good idea, I'd prefer to export function addHeaders from lib/http.js.

@MarhiievHE
Copy link
Member Author

OK, do I need to delete the last commit before pushing the new implementation? Or do I need to just make a new fixup for this commit: 2125df0?

@tshemsedinov
Copy link
Member

@MarhiievHE it is not important, I'll squash all commits to single landing this PR

@tshemsedinov
Copy link
Member

Landed in 9b96150

This pull request was closed.
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