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.

Sign up for GitHub

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

Jump to bottom

Upgrade gulp to version 5.0.0 #17922

Open
timvandermeij opened this issue Apr 11, 2024 · 3 comments
Open

Upgrade gulp to version 5.0.0 #17922

timvandermeij opened this issue Apr 11, 2024 · 3 comments
Labels
dependencies

Comments

@timvandermeij
Copy link
Contributor

timvandermeij commented Apr 11, 2024 •
edited

We should upgrade to Gulp 5 now that it's officially released. The changelog, including the breaking changes that mostly don't seem to affect us, can be found at https://github.com/gulpjs/gulp/releases/tag/v5.0.0.

I have already quickly tried the upgrade locally to see if it would be easy or not, and found that the main blocker appears to be the merge-stream package. It doesn't seem to work anymore with Gulp 5, causing tracebacks for most tasks (because of most of them directly or indirectly rely on merge calls).

The last commit for merge-stream, see https://github.com/grncdr/merge-stream, was 5 years ago, so this probably isn't very surprising. However, I played around with removing the merge calls altogether and didn't immediately find something that was obviously broken, so I'm mainly wondering what the use case of it is or was at the time of introduction (maybe it's obsolete nowadays, or something breaks when removing it that I just haven't noticed yet...). Sadly I haven't been able to find out more from e.g. Git commit messages.

We should investigate if merge-stream can be fixed, replaced or removed. Once that is addressed Gulp 5 should work, but note that we still need to carefully test all commands.
@Snuffleupagus

This comment was marked as outdated.

Sign in to view
@Snuffleupagus
Copy link
Collaborator

However, I played around with removing the merge calls altogether and didn't immediately find something that was obviously broken, so I'm mainly wondering what the use case of it is or was at the time of introduction (maybe it's obsolete nowadays, or something breaks when removing it that I just haven't noticed yet...).

Without some merge-like functionality I believe that we'll not correctly wait for everything to complete, which could possibly lead to intermittent bugs later on if we just remove it.

However, it seems that others have run into similar/same problems and according to gulpjs/gulp#2802 (comment) there's an "official" gulp-helper available; see https://www.npmjs.com/package/ordered-read-streams

timvandermeij added a commit to timvandermeij/pdf.js that referenced this issue May 22, 2024
@timvandermeij
Remove the merge-stream dependency in favor of the `ordered-read-st…
723e5b2 
…reams` dependency

The `merge-stream` dependency is no longer maintained and doesn't work
in combination with Gulp 5 anymore (for more information refer to
gulpjs/gulp#2802 (comment)).

Fortunately the Gulp team maintains a drop-in replacement dependency
called `ordered-read-streams` with the same API as `merge-stream`.
Indeed, running all affected Gulp targets and comparing build artifacts
with `diff -r <old> <new>` confirms that no unexpected changes are made.

Fixes a part of mozilla#17922.
@timvandermeij timvandermeij mentioned this issue May 22, 2024
Merged
@timvandermeij
Copy link
Contributor Author

Nice find; thanks! I have made the PR above to replace merge-stream, after which the Gulp 5 upgrade should be unblocked (provided that we don't find further defects while testing all Gulp targets carefully).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
dependencies
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@timvandermeij @Snuffleupagus