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

http2: add constant to already destructured constants #28176

Closed
wants to merge 2 commits into from

Conversation

@dnalborczyk
Copy link
Contributor

commented Jun 11, 2019

add constant to already destructured constants and while we're at it, destructure Readable and constants as well, remove binding.

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
lib/internal/http2/compat.js Outdated Show resolved Hide resolved
const Readable = Stream.Readable;
const binding = internalBinding('http2');
const constants = binding.constants;
const { Readable } = Stream;

This comment has been minimized.

Copy link
@lpinca

lpinca Jun 11, 2019

Member
Suggested change
const { Readable } = Stream;
const { Readable } = require('stream');

This comment has been minimized.

Copy link
@dnalborczyk

dnalborczyk Jun 11, 2019

Author Contributor

thing is that Http2ServerResponse extends from Stream below, so we need a Stream reference as well. if we could use ES6 modules internally, I'd use a 'default', and a 'named import': import Stream, { Readable } from ....

@@ -4,9 +4,8 @@ const { Object, ObjectPrototype, Reflect } = primordials;

const assert = require('internal/assert');
const Stream = require('stream');

This comment has been minimized.

Copy link
@lpinca

lpinca Jun 11, 2019

Member
Suggested change
const Stream = require('stream');

@dnalborczyk dnalborczyk force-pushed the dnalborczyk:destruct branch from 9979753 to d2a9fcf Jun 11, 2019

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

I am not a huge fan of changes like these. They do not seem to bring much benefit but I won't block this either.

@nodejs-github-bot

This comment was marked as outdated.

@Trott
Trott approved these changes Jul 8, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Landed in 3a9cb5c...b04de23

@Trott Trott closed this Jul 30, 2019

Trott added a commit to Trott/io.js that referenced this pull request Jul 30, 2019
http2: add constant to already destructured constants
PR-URL: nodejs#28176
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Trott added a commit to Trott/io.js that referenced this pull request Jul 30, 2019
http2: destructure constants from require call
PR-URL: nodejs#28176
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit that referenced this pull request Aug 2, 2019
http2: add constant to already destructured constants
PR-URL: #28176
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit that referenced this pull request Aug 2, 2019
http2: destructure constants from require call
PR-URL: #28176
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR referenced this pull request Aug 6, 2019
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: add constant to already destructured constants
PR-URL: nodejs#28176
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: destructure constants from require call
PR-URL: nodejs#28176
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: add constant to already destructured constants
PR-URL: nodejs#28176
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
http2: destructure constants from require call
PR-URL: nodejs#28176
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.