derhuerst / gulp-scale-images

Gulp plugin to resize each image into multiple smaller variants.
https://github.com/derhuerst/gulp-scale-images#gulp-scale-images
ISC License
13 stars 5 forks source link

Using a readMetadata within flatMap #30

Open IAluI opened 3 years ago

IAluI commented 3 years ago

My task is to get several images from one image using some configuration and metadata of the source file. I tried to solve this task by combining the 'flat-map' example and the readMetadata example given in the readme. I got something like this

const gulp = require('gulp');
const scaleImages = require('gulp-scale-images');
const flatMap = require('flat-map').default;

const createImages = (file, cb) => {
    readMetadata(file, (err, meta) => {
        if (err) return cb(err);
        file = file.clone();
        file.scale = {
            maxWidth: Math.floor(meta.width / 2),
            maxHeight: Math.floor(meta.height / 2),
        };
        cb(null, file);
    });
};

gulp.src('src')
    .pipe(flatMap(createImages))
    .pipe(scaleImages())
    .pipe(gulp.dest('dest'));

But if you need to process a lot of files, then some of them are lost in .pipe(flatMap(createImages)). More complex example https://github.com/IAluI/gulp-scale-images-issue.

While I was describing this issue, I found a working solution

const gulp = require('gulp');
const scaleImages = require('gulp-scale-images');
const flatMap = require('flat-map').default;

const createImages = (file, cb) => {
    cb(null, new Promise((resolve, reject) => {
        readMetadata(file, (err, meta) => {
            if (err) return reject(err);
            file = file.clone();
            file.scale = {
                maxWidth: Math.floor(meta.width / 2),
                maxHeight: Math.floor(meta.height / 2),
            };
            resolve(file);
        });
    }));
};

gulp.src('src')
    .pipe(flatMap(createImages))
    .pipe(scaleImages())
    .pipe(gulp.dest('dest'));

I understand that this problem is related to 'flat-map' and not to 'gulp-scale-images'. But since the readme says 'Combined with flat-map' it might be worth describing in the readme an example of how to use 'flat-map' with readMetadata?

derhuerst commented 3 years ago

My task is to get several images from one image [...].

If it is only ever one file that you want to convert, using Gulp is overkill, you can simplify your setup a lot. If there may be >1 files in the future, using Gulp is more appropriate.

I tried to solve this task by combining the 'flat-map' example and the readMetadata example given in the readme. I got something like this [...] While I was describing this issue, I found a working solution

In createImages, you pass a Promise, which will either resolve with the file's metadata & scaling instructions, or reject with an error, into cb. This does not make sense, you would either work with Promises everywhere (but Node.js streams don't support them in all cases), or you would use callbacks everywhere.

But if you need to process a lot of files, then some of them are lost in .pipe(flatMap(createImages)).

That definitely sounds like a bug, but it might be due to the way you use Promises in your example code.

More complex example https://github.com/IAluI/gulp-scale-images-issue.

build-images.js in your repo is not very readable unfortunately (e.g. it uses some gulp-load-plugins magic). Sorry, I don't have the time to look into why it doesn't work; Please provide a reduced example script that demonstrates the bug/issue.

I understand that this problem is related to 'flat-map' and not to 'gulp-scale-images'. But since the readme says 'Combined with flat-map' it might be worth describing in the readme an example of how to use 'flat-map' with readMetadata?

The readme does explain that, right? And last time I checked, example.js worked.

IAluI commented 3 years ago

Thanks for the quick response! And sorry about my awful english and confusing examples with errors.

I will try to explain the problem again =)

Of course I want to process a lot of files.

If I call cb argument of createImages within asynchronous closure, passing to cb an array of files, not all files are processed. At some point, the stream from .pipe (flatMap (createImages)) ends unexpectedly.

But if I pass Promise (that resolves with an array of files) to the cb argument of createImages all works well. I found the example of using flat-map with Promise in corresponding readme.

I simplified and fixed errors in my example repo.

You are right. Existing example works fine. But this is an example of synchronous file processing using flat-map. If using asynchronous processing (for example with readMetadata) and accessing cb through a closure, then it stops working.

I suggest adding an example of asynchronous processing. Perhaps making a mistake somewhere.

derhuerst commented 3 years ago

I simplified and fixed errors in my example repo.

Thanks, I can reproduce the issue.

There seems to be a problem with flat-map, because it works with a basic stream.Transform:

const {Transform} = require('stream');

const createImages = function (file, _, cb) {
    const that = this;
    readMetadata(file, (err, meta) => {
        if (err) {
            return cb(err);
        }

        const formats = [meta.format, 'webp'];
        const sizes = [200, 300, 400];
        const commonScale = {
            maxWidth: Math.trunc(meta.width * 0.5),
            withoutEnlargement: false,
            fit: 'cover',
            rotate: true,
            metadata: false,
        };
        formats.forEach((format) => {
            sizes.forEach((width) => {
                const img = file.clone();
                img.scale = {
                    ...commonScale,
                    format,
                    maxWidth: width,
                };
                img.dirname = `${img.dirname}/${width}`;
                that.push(img);
            });
        });

        cb();
    });
};

return gulp
    .src(`${SRC}/${IMG_GLOB}`)
    .pipe(new Transform({
        objectMode: true,
        transform: createImages,
    }))
    .pipe(scaleImages())
    .pipe(gulp.dest(`${DEST}/${IMG_ARRAY}`));

cc @rdy

derhuerst commented 2 years ago

I guess the best way to move forward is to submit a PR over at flat-map that reproduces the problem in a minimal way, and optionally even try to fix it.