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

Upgrade streamqueue to version 2.0.0 #18474

Closed
timvandermeij opened this issue Jul 21, 2024 · 5 comments · Fixed by #18483
Closed

Upgrade streamqueue to version 2.0.0 #18474

timvandermeij opened this issue Jul 21, 2024 · 5 comments · Fixed by #18483
Assignees

Comments

@timvandermeij
Copy link
Contributor

timvandermeij commented Jul 21, 2024

This dependency got updated after almost seven years, but likely because of that large timespan it also contains large changes. The changelog can be found at https://github.com/nfroidure/streamqueue/blob/main/CHANGELOG.md, but at this point in time it's blocked because it only supports Node >= 20 and currently we still support Node 18 LTS. This issue therefore tracks upgrading this dependency later once we dropped Node 18 support.

@Snuffleupagus
Copy link
Collaborator

At the risk of asking a stupid question, but could we perhaps replace this dependency with https://github.com/gulpjs/ordered-read-streams instead?

@timvandermeij
Copy link
Contributor Author

I'm not sure because I don't know too much about what streamqueue does exactly, but it's definitely something we should check out first because that might avoid having to do this (and also reduce the number of dependencies which can't hurt either).

@Snuffleupagus
Copy link
Collaborator

The reason that I hope that the existing ordered-read-streams package could be a drop-in replacement are the following observations.

  • Note this helper function for creating something from a string:

    pdf.js/gulpfile.mjs

    Lines 172 to 184 in 98e7727

    function createStringSource(filename, content) {
    const source = stream.Readable({ objectMode: true });
    source._read = function () {
    this.push(
    new Vinyl({
    path: filename,
    contents: Buffer.from(content),
    })
    );
    this.push(null);
    };
    return source;
    }

  • In at least one spot we're using that helper function as input to the ordered-function:

    pdf.js/gulpfile.mjs

    Lines 925 to 928 in 98e7727

    return ordered([
    createStringSource("locale.json", JSON.stringify(viewerOutput)).pipe(
    gulp.dest(VIEWER_LOCALE_OUTPUT)
    ),

  • Now note how similar the createStringSource and createTestSource helper functions look:

    pdf.js/gulpfile.mjs

    Lines 666 to 725 in 98e7727

    function createTestSource(testsName, { bot = false, xfaOnly = false } = {}) {
    const source = stream.Readable({ objectMode: true });
    source._read = function () {
    console.log();
    console.log("### Running " + testsName + " tests");
    const PDF_TEST = process.env.PDF_TEST || "test_manifest.json";
    let forceNoChrome = false;
    const args = ["test.mjs"];
    switch (testsName) {
    case "browser":
    if (!bot) {
    args.push("--reftest");
    } else {
    const os = process.env.OS;
    if (/windows/i.test(os)) {
    // The browser-tests are too slow in Google Chrome on the Windows
    // bot, causing a timeout, hence disabling them for now.
    forceNoChrome = true;
    }
    }
    if (xfaOnly) {
    args.push("--xfaOnly");
    }
    args.push("--manifestFile=" + PDF_TEST);
    break;
    case "unit":
    args.push("--unitTest");
    break;
    case "font":
    args.push("--fontTest");
    break;
    case "integration":
    args.push("--integration");
    break;
    default:
    this.emit("error", new Error("Unknown name: " + testsName));
    return null;
    }
    if (bot) {
    args.push("--strictVerify");
    }
    if (process.argv.includes("--noChrome") || forceNoChrome) {
    args.push("--noChrome");
    }
    if (process.argv.includes("--headless")) {
    args.push("--headless");
    }
    const testProcess = startNode(args, { cwd: TEST_DIR, stdio: "inherit" });
    testProcess.on("close", function (code) {
    if (code !== 0) {
    throw new Error(`Running ${testsName} tests failed.`);
    }
    source.push(null);
    });
    return undefined;
    };
    return source;
    }

  • Finally, note one example of how createTestSource is being using in:

    pdf.js/gulpfile.mjs

    Lines 1709 to 1714 in 98e7727

    return streamqueue(
    { objectMode: true },
    createTestSource("unit"),
    createTestSource("browser"),
    createTestSource("integration")
    );

@timvandermeij
Copy link
Contributor Author

Nice find! It'd require careful testing, but this does make sense and hopefully we can just replace the dependency then.

@timvandermeij
Copy link
Contributor Author

Unfortunately a replacement with ordered-read-streams doesn't work after trying this. While both return the output of the streams in order, the key difference between them turns out to be that streamqueue processes the readable streams one by one whereas ordered-read-streams processes all of them at once (thereby in our case triggering all test processes at once) and reorders the output internally to ensure an ordered return value. In other words, ordered-read-streams runs the input streams in parallel whereas streamqueue does so in series.

However, I think I have an alternative solution that would also get rid of streamqueue, while still running everything in series, that I'm trying out now.

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

Successfully merging a pull request may close this issue.

2 participants