Remove the through2 dependency in favor of the built-in Node.js stream.Transform
#18113
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.241.84.105:8877/d8444b3a78206c0/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/d8444b3a78206c0/output.txt Total script time: 1.17 mins Published |
…ream.Transform` The `through2` dependency got introduced over four years ago in mozilla#11325 to replace the unmaintained `gulp-transform` dependency. However, sadly the same holds for `through2` since the last release was also four years ago. Fortunately the `through2` dependency can trivially be replaced with the built-in Node.js `stream.Transform` API nowadays. In fact, the `through2` dependency mentions themselves in their README already that they are "a tiny wrapper around Node.js streams.Transform". The `stream.Transform` API is available in all Node.js versions we support, and in Node.js 6 already the simplified constructor approach for `stream.Transform` got introduced to simplify creating custom stream transformers; see https://nodejs.org/docs/latest-v6.x/api/stream.html#stream_new_stream_transform_options. This commit therefore replaces `through2` by switching to the `stream.Transform` API directly so we don't need any wrappers anymore. Note that for our case the only change we have to make is to enable object mode, see https://nodejs.org/api/stream.html#object-mode, because we pass in `VinylFile` objects instead of e.g. regular `Buffer` objects. I have confirmed in two ways that this is indeed a drop-in replacement: - Running the Gulp targets that call the `transform` function and diffing the resulting `build` folder before/after this patch, with `diff -r build-old/ build-new/`, to ensure that there are no unexpected changes in the output. - Changing the Gulpfile to, instead of UTF-8, transform the files to ASCII, and diffing the resulting `build` folder to confirm that the transformation logic works and produces different results, such as: ``` diff build/lib/core/standard_fonts.js build-ascii/lib/core/standard_fonts.js 284c284 < t["Trinité"] = true; --- > t["Trinit�"] = true; ```
timvandermeij
changed the title
though2 dependency in favor of the built-in Node.js stream.Transformthrough2 dependency in favor of the built-in Node.js stream.Transform
|
/botio integrationtest |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @timvandermeij received. Current queue size: 0 Live output at: http://54.241.84.105:8877/9e7299f59c6d0d1/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @timvandermeij received. Current queue size: 0 Live output at: http://54.193.163.58:8877/e9a1d674a86bf6c/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/9e7299f59c6d0d1/output.txt Total script time: 7.36 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/e9a1d674a86bf6c/output.txt Total script time: 21.94 mins
|
Snuffleupagus
approved these changes
May 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
through2dependency got introduced over four years ago in #11325 to replace the unmaintainedgulp-transformdependency. However, sadly the same holds forthrough2since the last release was also four years ago.Fortunately the
through2dependency can trivially be replaced with the built-in Node.jsstream.TransformAPI nowadays. In fact, thethrough2dependency mentions themselves in their README already that they are "a tiny wrapper around Node.js streams.Transform". Thestream.TransformAPI is available in all Node.js versions we support, and in Node.js 6 already the simplified constructor approach forstream.Transformgot introduced to simplify creating custom stream transformers; see https://nodejs.org/docs/latest-v6.x/api/stream.html#stream_new_stream_transform_options.This commit therefore replaces
through2by switching to thestream.TransformAPI directly so we don't need any wrappers anymore. Note that for our case the only change we have to make is to enable object mode, see https://nodejs.org/api/stream.html#object-mode, because we pass inVinylFileobjects instead of e.g. regularBufferobjects.I have confirmed in two ways that this is indeed a drop-in replacement:
transformfunction and diffing the resultingbuildfolder before/after this patch, withdiff -r build-old/ build-new/, to ensure that there are no unexpected changes in the output.buildfolder to confirm that the transformation logic works and produces different results, such as:For reviewing I recommend to use the
?w=1flag since it makes the diff even shorter.