Skip to content
This repository has been archived by the owner on Jun 21, 2022. It is now read-only.

bug 1333120: fix headers in kumascript response #106

Merged
merged 1 commit into from Feb 7, 2017
Merged

bug 1333120: fix headers in kumascript response #106

merged 1 commit into from Feb 7, 2017

Conversation

escattone
Copy link
Contributor

Background
When we upgraded Node.js to 6.9.2 (#69) , we also upgraded to version 4 of express. Version 0.1.0 (the only version) of the firelogger node module does not work with the latest API's of express' Request and Response objects, so it was causing problems (e.g., the "Vary" header was no longer correct).

git comments

  • the firelogger npm module 0.1.0 does not work
    with version 4 of express (firelogger has not
    been updated in 5+ years)
  • pull firelogger.js into the repo, update the
    request and response objects to use the version
    4 express API's, and fix for clean pass of jshint
  • remove firelogger from the package.json and
    create a new npm-shrinkwrap.json
  • fix broken test in test-server.js that should
    have detected this issue immediately

* the firelogger npm module 0.1.0 does not work
  with version 4 of express (firelogger has not
  been updated in 5+ years)
* pull firelogger.js into the repo, update the
  request and response objects to use the version
  4 express API's, and fix for clean pass of jshint
* remove firelogger from the package.json and
  create a new npm-shrinkwrap.json
* fix broken test in test-server.js that should
  have detected this issue immediately
@escattone
Copy link
Contributor Author

FYI. I also submitted a PR to the firelogger repo. If that is accepted and a new version of the module uploaded to npm, we can consider using that instead of the copy we pulled into our repo.

@@ -0,0 +1,83 @@
/*jshint node: true*/
Copy link
Contributor Author

@escattone escattone Feb 6, 2017

Choose a reason for hiding this comment

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

FYI. This file is a copy of https://github.com/lmorchard/node-firelogger/blob/master/firelogger.js but updated to use the latest methods of the Request and Response objects of express (specifically, the get and set methods). Also this comment has been added to the top of the file to suppress "implied global module" errors when running jshint.

Copy link
Contributor

@jwhitlock jwhitlock left a comment

Choose a reason for hiding this comment

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

👍 the headers are fixed when running curl -I http://localhost:9080/docs/en-US/HTML
👍 improved header tests
👎 npm modules, whose authors don't both to maintain a changelog

@jwhitlock jwhitlock merged commit 6854ee1 into mdn:master Feb 7, 2017
jwhitlock added a commit to mdn/kuma that referenced this pull request Feb 7, 2017
* Fix headers in kumascript response
  mdn/kumascript#106
@lmorchard
Copy link
Contributor

FWIW, there's no changelog on node-firelogger, because there was only ever one version :)

@jwhitlock
Copy link
Contributor

You're in good company. There were about 10 other node packages updated as well, and I stopped checking after 3 packages without changelogs.

@escattone escattone deleted the fix-headers-1333120 branch April 12, 2017 18:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants