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

Do not use the deprecated Buffer() constructor #304

Closed
wants to merge 2 commits into from

Conversation

lpinca
Copy link

@lpinca lpinca commented Oct 14, 2018

@phated phated added this to TODO in v4 Dec 7, 2018
@lpinca
Copy link
Author

lpinca commented Apr 14, 2019

@phated are you interested in this? If so I will rebase. If not I will close.

@phated
Copy link
Member

phated commented Apr 19, 2019

@lpinca I'm going to be tweaking stuff in the next week or so. It looks like there are some file conflicts right now.

@lpinca
Copy link
Author

lpinca commented Apr 20, 2019

It looks like there are some file conflicts right now.

Yes, I asked exactly for that reason. There is no point in resolving conflicts if the patch is not wanted.

@BurtHarris
Copy link
Contributor

Seems like this is languishing due to a very narrow test not failing in the expected way! An assert IS generated is appveyor test environment (not travis-ci) running Node 0.12, perhaps the test is too specific as to what is expected in a negative test.

What is the point of supporting Node.js version all the way back to version 0. I somehow doubt anyone is seriously interested in building systems that depend on that platform, particularly on Windows.

Stack for that failure suggests to me the bug is in the test. It says:


  1. closeFd calls the callback with close error if no error to propagate:
    Uncaught Error: Expected undefined to exist
    at assert (node_modules\expect\lib\assert.js:29:9)
    at Expectation.toExist (node_modules\expect\lib\Expectation.js:52:28)
    at test\file-operations.js:733:19
    at onClosed (lib\file-operations.js:26:5)
    at node_modules\graceful-fs\graceful-fs.js:45:10
    at FSReqWrap.oncomplete (fs.js:95:15)

@phated
Copy link
Member

phated commented Jul 2, 2019

@BurtHarris it's not been looked at because I started a new job that's taking most of my time and traveling which is taking the rest. And I'm the only one "actively" working on this stuff.

Thanks for diving in further, but I believe that test was a regression test so we should probably figure out why it is breaking (though it probably isn't happening from this PR???).

@phated
Copy link
Member

phated commented Jul 2, 2019

@sttk would you have some time to debug that test?

@sttk
Copy link
Contributor

sttk commented Jul 4, 2019

@phated I could reproduce this error on appveyor but not on my Windows environment. This error may be appveyor specific and on my environment. I'll dig into detail about this.

@sttk
Copy link
Contributor

sttk commented Jul 6, 2019

This problem on v0.12 is the known issue as per nodejs/node-v0.x-archive#4574 or nodejs/node#3718.
And this problem happens on node.js v0.11, v0.12 and io.js.
So the target test case should be skipped if node version > 0.10 and < 4.

@phated phated removed this from TODO in v4 Apr 28, 2020
@phated phated added this to To do in v5 Apr 28, 2020
@phated phated removed this from To do in v5 Apr 28, 2020
@phated
Copy link
Member

phated commented May 14, 2020

Since we are dropping node <10.13.0 support, we will be able to use the buffer constructor directly.

@phated phated closed this May 14, 2020
@phated phated mentioned this pull request May 14, 2020
@lpinca lpinca deleted the fix/deprecation-warning branch May 14, 2020 05:43
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.

None yet

5 participants