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 [DEP0066] warning for node > 10.x #367

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

FlorentinTh
Copy link

Just a simple change from : this._headers to this.getHeaders() in line 18 of response.js. It should be enough to remove the warning [DEP0066] DeprecationWarning: OutgoingMessage.prototype._headers is deprecated for users of Node > 10.x

@masx200
Copy link

masx200 commented Apr 24, 2020

Support it. Great. Well done.

@FlorentinTh
Copy link
Author

FlorentinTh commented Apr 24, 2020

Yeah but it needs improvement to support node < 7.x
I found the solution!

@FlorentinTh
Copy link
Author

I added a ternary variable assignment to check either the new response.getHeaders() is available (for node >= 7.x) or not (node < 7.x). In the last case, it still using the old response.headers

@masx200
Copy link

masx200 commented Jun 8, 2020

@LuigiMaestrelli
Copy link

Any news on this issue?
Thanks

@FlorentinTh
Copy link
Author

Nope. No one seems to be available to help on tests with node > 10 that I don’t have time to go deeper in to understand. Moreover they have to allow pull request also with no one responding...

@franz-josef-kaiser
Copy link

@daviddias Can you please take a look and invest 5 mins of your time here? It's an easy fix and people really need it to move forward. Thanks a lot, it will be highly appreciated!

@daviddias
Copy link
Member

Thanks for the nudge @franz-josef-kaiser. I'm not actively maintaining this package for a very long time, having passed that baton to @jacobheun. Let me ping him so that he can review it better :)

@attila-boros
Copy link

Hi @FlorentinTh, thanks for the fix!

I ran test/client-test.js separately with

npm exec mocha test/client-test.js

on Node v14.17.4 (LTS) and noticed that POST requests and their "after each" hooks are failing in SSL mode. If I move the it('should send GET request' test after it('should send POST request', the tests are passing. So GET after POST is working, but the other way it isn't. In general the order in which the tests are executed should not matter, probably some resource is not cleaned up properly.

I hope the maintainers can take a look at this.

@FlorentinTh
Copy link
Author

Nice work @attila-boros.
At least you figured out the issue with failing tests giving less work to the maintainers.
I hope so the PR will be merged and released one day.

Copy link

@lamweili lamweili left a comment

Choose a reason for hiding this comment

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

Closes #366, Closes #382

@lamweili
Copy link

lamweili commented Oct 20, 2021

@indutny Similar to spdy-http2/http-deceiver#7 (comment), can you review this as well?

/cc @alexander-akait

@ZLester
Copy link

ZLester commented Dec 10, 2021

@jacobheun @daviddias Any news on this one?

@lamweili
Copy link

lamweili commented Jun 9, 2022

cc: @jacobheun @daviddias @indutny

@nickscialli-msft
Copy link

nickscialli-msft commented Dec 2, 2022

Wanted to bump this as well as I'm getting warnings on my http2/express server using the node-spdy package!

Also, does anyone on this thread know the potential effects of this deprecation? Can it affect/drop headers or is more along the lines of using an existing (but unsupported) API? Thanks!

@beenotung
Copy link

@indutny would you take a few minutes to review this PR or grant some of us the permission to accept PR (and publish on npm)?

Otherwise I'm willing to take the load to fork this package and get this PR published to npm. Appreciate the contribution from everyone 🙏

beenotung added a commit to beenotung/node-spdy that referenced this pull request Dec 12, 2022
@beenotung
Copy link

I've forked this package into spdy-fixes to get the fixes published to npm. I'm happy to merge the fixes upstream if the author return to this project.

I may try to look into other issues and pull request upon suggestions. Appreciate the contributions from everyone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.