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

Fix SPDY fails in node >= 11.1.0 #1660

Merged
merged 4 commits into from
Feb 18, 2019
Merged

Fix SPDY fails in node >= 11.1.0 #1660

merged 4 commits into from
Feb 18, 2019

Conversation

yi-ge
Copy link
Contributor

@yi-ge yi-ge commented Feb 11, 2019

In node v11+:

util.js:305
    throw new ERR_INVALID_ARG_TYPE('superCtor.prototype',
    ^

TypeError [ERR_INVALID_ARG_TYPE]: The "superCtor.prototype" property must be of type Object. Received type undefined
  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

In node v11+:
```
util.js:305
    throw new ERR_INVALID_ARG_TYPE('superCtor.prototype',
    ^

TypeError [ERR_INVALID_ARG_TYPE]: The "superCtor.prototype" property must be of type Object. Received type undefined
```
@jsf-clabot
Copy link

jsf-clabot commented Feb 11, 2019

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@yi-ge
Copy link
Contributor Author

yi-ge commented Feb 11, 2019

About it: https://github.com/spdy-http2/node-spdy/issues/350

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

We need add tests

lib/Server.js Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 11, 2019

Codecov Report

Merging #1660 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1660      +/-   ##
==========================================
- Coverage   75.12%   75.08%   -0.05%     
==========================================
  Files          18       18              
  Lines         591      590       -1     
  Branches      171      171              
==========================================
- Hits          444      443       -1     
  Misses        113      113              
  Partials       34       34
Impacted Files Coverage Δ
lib/Server.js 78.43% <100%> (-0.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f78a9a3...6a655ec. Read the comment docs.

@alexander-akait
Copy link
Member

Also please accept CLA

@yi-ge
Copy link
Contributor Author

yi-ge commented Feb 11, 2019

t.js:

const spdy = require('spdy');

You can run it (node t.js) in node v11.9.0.
image

About: spdy-http2/node-spdy#350

@yi-ge
Copy link
Contributor Author

yi-ge commented Feb 11, 2019

😄

@alexander-akait
Copy link
Member

@yi-ge looks you catch the bug in nodejs 😄 Can you create issue in nodejs and add comment with link in source code

@yi-ge
Copy link
Contributor Author

yi-ge commented Feb 11, 2019

@evilebottnawi Thank you.

@alexander-akait
Copy link
Member

Strange can't reproducible locally on latest nodejs

@yi-ge
Copy link
Contributor Author

yi-ge commented Feb 15, 2019

@evilebottnawi sdpy v4.0?

Node.js 11.9.0, spdy 4.0.

@alexander-akait
Copy link
Member

alexander-akait commented Feb 15, 2019

@yi-ge

Now using node v11.9.0 (npm v6.5.0)

can we add tests for this case and looks what CI show?

@yi-ge
Copy link
Contributor Author

yi-ge commented Feb 17, 2019

@evilebottnawi It's just a line of code.

@yi-ge
Copy link
Contributor Author

yi-ge commented Feb 17, 2019

image
vue cli create project.

@yi-ge
Copy link
Contributor Author

yi-ge commented Feb 17, 2019

#1592

@alexander-akait
Copy link
Member

@yi-ge Strange, can't reproduce:

node -v
v11.9.0

Input:

const spdy = require('spdy');

console.log(spdy);

Output:

node test.js

{ handle: { [Function: Handle] create: [Function: create] },
  request: { onNewListener: [Function: onNewListener] },
  response:
   { writeHead: [Function: writeHead],
     end: [Function: end],
     push: [Function: push],
     writeContinue: [Function: writeContinue] },
  Socket: [Function: Socket],
  agent:
   { Agent: { [Function: Agent] create: [Function: create] },
     PlainAgent: { [Function: Agent] create: [Function: create] },
     create: [Function: create] },
  Agent: { [Function: Agent] create: [Function: create] },
  createAgent: [Function: create],
  server:
   { Server: { [Function: Server] create: [Function: create] },
     PlainServer: { [Function: Server] create: [Function: create] },
     create: [Function: create] },
  Server: { [Function: Server] create: [Function: create] },
  PlainServer: { [Function: Server] create: [Function: create] },
  createServer: [Function: create] }

What os you use?

@yi-ge
Copy link
Contributor Author

yi-ge commented Feb 18, 2019

@evilebottnawi MacOS. Merging code does not affect the result. You can first merge and verify. 😄

@alexander-akait
Copy link
Member

@yi-ge So this PR doesn't solve problem?

@yi-ge
Copy link
Contributor Author

yi-ge commented Feb 18, 2019

@evilebottnawi This OR can solve the problem.

@yi-ge
Copy link
Contributor Author

yi-ge commented Feb 18, 2019

@evilebottnawi This PR, No consequences. 😄

@alexander-akait
Copy link
Member

@yi-ge i can't understand you, i can't reproduce problem 😞 Maybe it is happens only on macos?

@yi-ge
Copy link
Contributor Author

yi-ge commented Feb 18, 2019

image

@evilebottnawi Only on macos, maybe. 😂

@alexander-akait
Copy link
Member

@yi-ge looks like all works fine, i can't understand problem, please clarrify

@yi-ge
Copy link
Contributor Author

yi-ge commented Feb 18, 2019

@evilebottnawi The same code cannot be run in Mac OS node v11.1.0 +. You can see: spdy-http2/node-spdy#350 or #1592. This PR avoid this kind of situation from happening.

@alexander-akait
Copy link
Member

@yi-ge thanks for clarify, just add comment in code with link on issue and we can merge this 👍

@alexander-akait alexander-akait merged commit b92e5fd into webpack:master Feb 18, 2019
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.

3 participants