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: Replace nextTick by setImmediate. #360

Merged
merged 4 commits into from
Oct 11, 2024
Merged

Conversation

regseb
Copy link
Contributor

@regseb regseb commented Aug 1, 2022

Replace process.nextTick() by setImmediate() to be closer to the reality of asynchronous methods. This pull request fixes #352 testcase with yauzl.

@3cp
Copy link
Collaborator

3cp commented Aug 2, 2022

The CI shows this fix only works for Nodejs v16+.
You can try to add a condition to test against nodejs version.

@regseb
Copy link
Contributor Author

regseb commented Aug 2, 2022

I replaced the nextTick only for the promise and it seems to work on all versions of Node. Maybe the callbacks are executed directly after, while the promises are executed on the next event loop.

@3cp
Copy link
Collaborator

3cp commented Aug 3, 2022

@tschaub can you review this? I am not sure about the side effect of this change.

@3cp 3cp requested a review from tschaub August 13, 2022 11:37
@regseb
Copy link
Contributor Author

regseb commented Oct 11, 2024

I tested mock-fs with the latest version 5.3.0 and the problem is still there.

  • index.js

    import mock from "mock-fs";
    import yauzl from "yauzl";
    
    if (!process.argv.includes("without-mock")) {
        mock({ "foo.zip": mock.load("foo.zip") });
    }
    
    // Test with setImmediate.
    yauzl.open("foo.zip", (err, zipfile) => {
        if (err) throw err;
        setImmediate(() => {
            zipfile.on("error", (err) => console.log("[setImmediate] error", err));
            zipfile.on("entry", (entry) => console.log("[setImmediate] entry", entry.fileName));
            zipfile.on("end", () => console.log("[setImmediate] end"));
        });
    });
    
    // Wait one second between the two tests.
    await new Promise((r) => setTimeout(r, 1000));
    
    // Test with Promise.
    const zipfile = await new Promise((resolve, reject) => {
        yauzl.open("foo.zip", (err, zipfile) => {
            if (err) reject(err);
            resolve(zipfile);
        });
    });
    zipfile.on("error", (err) => console.log("[Promise] error", err));
    zipfile.on("entry", (entry) => console.log("[Promise] entry", entry.fileName));
    zipfile.on("end", () => console.log("[Promise] end"));
  • package.json

    {
        "name": "testcase",
        "version": "1.0.0",
        "type": "module",
        "dependencies": {
            "mock-fs": "5.3.0",
            "yauzl": "3.1.3"
        }
    }
  1. npm install
  2. zip foo.zip package.json (or zip package.json with an other tool)
  3. node index.js
    • Current:

      [setImmediate] end
      [Promise] end

    • Expected:

      [setImmediate] entry package.json
      [setImmediate] end
      [Promise] entry package.json
      [Promise] end


@tschaub Can you review this PR to fix the problem?

@tschaub tschaub merged commit c83e33e into tschaub:main Oct 11, 2024
12 checks passed
@regseb regseb deleted the promise branch October 11, 2024 17:21
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.

Different behavior with yauzl
3 participants