FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http: replace superfluous property with getter/setter by ronag · Pull Request #29015 · nodejs/node · GitHub

/ node Public

http: replace superfluous property with getter/setter - #29015

Closed
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:http-connection
Closed

http: replace superfluous property with getter/setter#29015
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:http-connection

Conversation

ronag commented Aug 6, 2019
edited
Loading

Copy link
Copy Markdown
Member

Slightly reduces memory overhead by replacing superfluous property with getter/setter.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Aug 6, 2019
ronag changed the title http: replace duplicate property with getter/setter http: replace superfluous property with getter/setter Aug 6, 2019
ronag force-pushed the http-connection branch from 3300437 to 73d0979 Compare August 6, 2019 11:58

mscdex commented Aug 6, 2019

Copy link
Copy Markdown
Contributor

AFAIK getters/setters like this incur noticeable overhead, so perhaps we should be adjusting the documentation to more strongly recommend socket over connection.

mscdex commented Aug 6, 2019

Copy link
Copy Markdown
Contributor

Also if we're making this change we should also replace all usage of .connection in core with .socket to avoid the overhead.

ronag force-pushed the http-connection branch 2 times, most recently from 551d5ec to 9fe52d4 Compare August 6, 2019 12:51

ronag commented Aug 6, 2019

Copy link
Copy Markdown
Member Author

@mscdex better?

ronag force-pushed the http-connection branch 7 times, most recently from c16f5f6 to 80a07da Compare August 6, 2019 13:56
Comment thread doc/api/deprecations.md Outdated
Comment thread doc/api/deprecations.md Outdated
Comment thread doc/api/deprecations.md Outdated
Comment thread doc/api/deprecations.md Outdated
ronag force-pushed the http-connection branch from 80a07da to f9f6147 Compare August 6, 2019 14:57
jasnell added deprecations Issues and PRs related to deprecations. semver-major PRs that contain breaking changes and should be released in the next major version. labels Aug 7, 2019
ronag force-pushed the http-connection branch from f9f6147 to ddc441f Compare August 9, 2019 16:03

ronag commented Aug 17, 2019

Copy link
Copy Markdown
Member Author

@Trott: this seems ready

Copy link
Copy Markdown
Collaborator

Trott commented Aug 17, 2019

Copy link
Copy Markdown
Member

Landed in 0daec61

Trott closed this Aug 17, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deprecations Issues and PRs related to deprecations. http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants


Back | FazBrowse Home | New Git URL