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

Refactor http server; support content-disposition #1039

Merged
merged 1 commit into from Feb 10, 2017
Merged

Conversation

@feross
Copy link
Member

feross commented Feb 10, 2017

Refactored the server into many smaller functions to make it easier to
understand all the different code paths.

  • added a Content-Disposition header, which tells the browser the
    file's name, since we use urls like http://localhost:port/0 <-- no
    human-readable file name
  • Server returns valid HTML documents (with all the required tags) now.
  • Return 204 status for OPTIONS request
  • reduce access-control-max-age to chromium max of 600s
  • respond to OPTIONS requests that lack
    'access-control-request-headers' (before they were treated as GET)
  • return '405 invalid verb' for all other verbs

For: brave/browser-laptop#6737

Refactored the server into many smaller functions to make it easier to
understand all the different code paths.

- added a Content-Disposition header, which tells the browser the
file's name, since we use urls like http://localhost:port/0 <-- no
human-readable file name
- Server returns valid HTML documents (with all the required tags) now.
- Return 204 status for OPTIONS request
- reduce access-control-max-age to chromium max of 600s
- respond to OPTIONS requests that lack
'access-control-request-headers' (before they were treated as GET)
- return '405 invalid verb' for all other verbs

For: brave/browser-laptop#6737
@feross

This comment has been minimized.

Copy link
Member Author

feross commented Feb 10, 2017

Post-merge review welcome 🖐 Merging and releasing because I need this to fix a bug in Brave.

@feross feross merged commit 22d7dd5 into master Feb 10, 2017
5 checks passed
5 checks passed
Node Security No known vulnerabilities found
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@feross feross deleted the content-disposition branch Feb 10, 2017
@feross

This comment has been minimized.

Copy link
Member Author

feross commented Feb 10, 2017

0.98.6

Copy link
Member

DiegoRBaquero left a comment

LGTM

@feross

This comment has been minimized.

Copy link
Member Author

feross commented Feb 10, 2017

Thanks @DiegoRBaquero!

@feross feross mentioned this pull request Feb 10, 2017
4 of 4 tasks complete
@lock

This comment has been minimized.

Copy link

lock bot commented May 4, 2018

This thread has been automatically locked because it has not had recent activity. To discuss futher, please open a new issue.

@lock lock bot locked as resolved and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.