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

Not working with Node 14 #94

Closed
giggio opened this issue Apr 22, 2020 · 11 comments
Closed

Not working with Node 14 #94

giggio opened this issue Apr 22, 2020 · 11 comments

Comments

@giggio
Copy link

giggio commented Apr 22, 2020

It seems that this library is not working with Node.js 14, that just came out. It seems that Node.js crashes exactly at this line (stream.pipline):

https://github.com/maxogden/extract-zip/blob/eb3c1edb8481bbf68da05cd3a824b1dcc697f908/index.js#L135

I am not sure if they changed something, and this is a breaking change and something needs to be updated on extract-zip, or if this is a bug on Node.js 14, that just came out. Anyway, it is breaking, and the issue is here.

@nstepien
Copy link

Can confirm, it's silently breaking puppeteer install script on node 14+win10 at least.

@AviVahl
Copy link

AviVahl commented Apr 23, 2020

Will probably be fixed by nodejs/node#32968

A change in Node's stream behavior caused several stream based libs to break, including tar-stream. Wouldn't surprise me if this is the same issue.

@aduh95
Copy link
Contributor

aduh95 commented Apr 23, 2020

I pulled the Node's PR and built it locally, unfortunately the tests are still failing...

@ronag
Copy link

ronag commented Apr 25, 2020

I'm not quite familiar with this library. Could someone write a full repro example and I will take a look asap.

@ronag
Copy link

ronag commented Apr 25, 2020

All extract-zip tests succeed. Do you think you could make a failing test which we can use to resolve this?

@ronag
Copy link

ronag commented Apr 25, 2020

This works on Node master:

const extract = require('./index')

async function main() {
    try {
        await extract('./Archive.zip', { dir: '/Users/ronagy/GitHub/public/extract-zip/sasd' })
        console.log('Extraction complete')
    } catch (err) {
        console.error(err)
        // handle any errors
    }
}
main()

@ronag
Copy link

ronag commented Apr 25, 2020

I believe this might have been fixed through nodejs/node#32967

@aduh95
Copy link
Contributor

aduh95 commented Apr 25, 2020

All extract-zip tests succeed.

@ronag that's not what I see. Using Node.js 14.0.0, running yarn ava:

yarn run v1.22.4
(node:53652) [DEP0139] DeprecationWarning: Calling process.umask() with no arguments is prone to race conditions and is a potential security vulnerability.
(Use `node --trace-deprecation ...` to show where the warning was created)
$ ava

⠹ (node:53654) Warning: Accessing non-existent property 'levels' of module exports inside circular dependency
(Use `node --trace-warnings ...` to show where the warning was created)
⠴ extract broken zip

  6 tests failed

  verify github zip extraction worked


  Error: Promise returned by test never resolved



  opts.onEntry


  Error: Promise returned by test never resolved



  defaultDirMode


  Error: Promise returned by test never resolved



  defaultFileMode not set


  Error: Promise returned by test never resolved



  defaultFileMode


  Error: Promise returned by test never resolved



  files in subdirs where the subdir does not have its own entry is extracted


  Error: Promise returned by test never resolved

error Command failed with exit code 1.

Because of some engine restrictions when using Node 15 nightly with yarn, the exact command I type to test the PR you mentioned is ../node/out/Release/node node_modules/.bin/ava. Same output as Node 14 btw.

Anyway, I believe Michaël came up with a probably simpler repro on the other thread.

@ronag
Copy link

ronag commented Apr 25, 2020

So the problem here is that zipFile.openReadStream for some reason never emits 'close' even though autoDestroy is enabled on the stream. It's probably overriding some stream internals. Will see if we can find a way to detect this. Not sure how actively maintained yauzl is and whether it would make sense to try to fix it there.

@aduh95
Copy link
Contributor

aduh95 commented Apr 27, 2020

The fix has been merged on Node.js repo, I can confirm extract-zip's tests are now passing. I believe we can close this issue as soon as Node.js 14.1.0 is released.

@giggio
Copy link
Author

giggio commented May 1, 2020

Just tested it on Node.js 14.1.0 and it is fixed.

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 a pull request may close this issue.

5 participants