Enable relative paths for cache keys

levacic commented

2018-03-05 12:51:57 +00:00

(Migrated from github.com)

A use-case we have when optimizing images is a nested folder structure, where the same filename might appear in different folders.

This is a problem for this library because the cache keys only use the basename, so it incorrectly tracks such files.

One simple (hacky) change that fixes this is the following:

-var filename = path.basename(file.path),
+var filename = path.relative('.', file.path),

This results in the cache keys being relative from the project root (ie. from where Gulp is being run), and is the correct thing to do in our case.

Would you be open to a PR that enables something like this via an option? Perhaps it should be possible to also base a base path from which the relative path should be resolved?

If you like the suggestion, and if you have any thoughts on how you'd like this implemented, or a better idea, let me know and I can do it.

A use-case we have when optimizing images is a nested folder structure, where the same filename might appear in different folders. This is a problem for this library because the cache keys only use the basename, so it incorrectly tracks such files. One simple (hacky) change that fixes this is the following: ```diff -var filename = path.basename(file.path), +var filename = path.relative('.', file.path), ``` This results in the cache keys being relative from the project root (ie. from where Gulp is being run), and is the correct thing to do in our case. Would you be open to a PR that enables something like this via an option? Perhaps it should be possible to also base a base path from which the relative path should be resolved? If you like the suggestion, and if you have any thoughts on how you'd like this implemented, or a better idea, let me know and I can do it.

corneliusio commented

2018-03-05 14:53:44 +00:00

(Migrated from github.com)

I do agree there needs to be more flexibility in this area, I appreciate you bringing this suggestion up.

This plugin has been long overdue for me to go in and make some updates/refactor a few things (not the least of which is to remove the 'gulp-util'). I've mostly put it off because it doesn't seem to get that much usage and I'm using it in a very limited scope now.

I should actually have time to jump in and do this today. I'm definitely open to using relative paths for cache keys and adding a context option to work from relatively. I'm curious, in your case, is the 'namespace' option not a feasible solution (I can definitely think of situations where it wouldn't be) and if not, if the 'namespace' option was a function that was passed the vinyl file object and then returned a namespace key based on any custom logic you provided, would that be workable for you?

I do agree there needs to be more flexibility in this area, I appreciate you bringing this suggestion up. This plugin has been long overdue for me to go in and make some updates/refactor a few things (not the least of which is to remove the 'gulp-util'). I've mostly put it off because it doesn't seem to get that much usage and I'm using it in a very limited scope now. I should actually have time to jump in and do this today. I'm definitely open to using relative paths for cache keys and adding a context option to work from relatively. I'm curious, in your case, is the 'namespace' option not a feasible solution (I can definitely think of situations where it wouldn't be) and if not, if the 'namespace' option was a function that was passed the vinyl file object and then returned a namespace key based on any custom logic you provided, would that be workable for you?

levacic commented

2018-03-05 16:44:01 +00:00

(Migrated from github.com)

As I understood correctly, the namespace option is there if someone wants to, for example, separately track caches for scripts and styles - but within a namespace, the files processed would still only have the cache keyed by the basename() which isn't ideal in cases where multiple files with the same name may exist in different paths.

The workaround with the namespace function could be a good solution, as we could, for example, return the path to the file (minus the actual filename) as the namespace (e.g. a namespace for each folder), or even the full file path could be a namespace (e.g. a namespace for each file) - it doesn't really matter.

Thanks for the very quick response!

EDIT: BTW, I'm not sure why, but among the many caching plugins for Gulp, I'm pretty sure this is the only one I was able to find that natively does built-in caching across Gulp runs - most of them are just meant to be used with watch-style tasks, and I've found a few where a cache object can be passed on initialization and retrieved after completion, which still requires wiring up some additional code to persist the cache. It's a bit surprising that this isn't a more common use-case. We are using this for optimizing images via imagemin, and are even considering committing the cache storage file to the repo, so that caching would work team-wide.

As I understood correctly, the namespace option is there if someone wants to, for example, separately track caches for scripts and styles - but within a namespace, the files processed would still only have the cache keyed by the `basename()` which isn't ideal in cases where multiple files with the same name may exist in different paths. The workaround with the `namespace` function could be a good solution, as we could, for example, return the path to the file (minus the actual filename) as the namespace (e.g. a namespace for each folder), or even the full file path could be a namespace (e.g. a namespace for each file) - it doesn't really matter. Thanks for the very quick response! EDIT: BTW, I'm not sure why, but among the many caching plugins for Gulp, I'm pretty sure this is the only one I was able to find that natively does built-in caching across Gulp runs - most of them are just meant to be used with watch-style tasks, and I've found a few where a cache object can be passed on initialization and retrieved after completion, which still requires wiring up some additional code to persist the cache. It's a bit surprising that this isn't a more common use-case. We are using this for optimizing images via `imagemin`, and are even considering committing the cache storage file to the repo, so that caching would work team-wide.

corneliusio commented

2018-03-05 21:12:51 +00:00

(Migrated from github.com)

Ha yeah, that was exactly why I wrote it. Specifically, for preventing extra calls to the tinypng api since I didn't want to eat up my free quota every time I booted gulp up. I do actually commit the .checksum file for that very reason. Though I'm not in a team setting, I do work from multiple devices so this can be pretty big help.

All that to say, I've published an update that adds a context option. Still have to add it to the readme, but after getting in there and seeing what all I'd want to change long-term, decided to go ahead and publish this little "workaround" for the time being to get you going.

By default, the plugin will continue to operate as it has been. But, if you provide a context, all of the checksum keys will be the filename and path relative to the context.

gulp.src('src/**/*')
    .pipe(once({context: process.cwd()}))
    .pipe(gulp.dest('dist'));

Hope this helps!

Ha yeah, that was exactly why I wrote it. Specifically, for preventing extra calls to the tinypng api since I didn't want to eat up my free quota every time I booted gulp up. I do actually commit the `.checksum` file for that very reason. Though I'm not in a team setting, I do work from multiple devices so this can be pretty big help. All that to say, I've published an update that adds a `context` option. Still have to add it to the readme, but after getting in there and seeing what all I'd want to change long-term, decided to go ahead and publish this little "workaround" for the time being to get you going. By default, the plugin will continue to operate as it has been. But, if you provide a context, all of the checksum keys will be the filename and path relative to the context. ```js gulp.src('src/**/*') .pipe(once({context: process.cwd()})) .pipe(gulp.dest('dist')); ``` Hope this helps!

levacic commented

2018-03-05 21:54:53 +00:00

(Migrated from github.com)

Huh, so it seems I'm having some trouble running the code - I was on version 1.0.2 previously and updating to 1.2.0 stopped the plugin from working - so I tested 1.1.0 and it isn't working either for me (I'm on Node v6.11.4).

So I've literally gone line by line through the diff between 1.0.2 and 1.1.0, starting with 1.0.2 and replacing everything to match your changes - and the plugin stops working for me after this change near the bottom of index.js:

-        }
 
-        flow.push(file);
-        return next();
+            return next(null, file);
+        }

Any ideas what could cause this?

P.S. I'm looking at this diff: github.com/corneliusio/gulp-once@d37691e2df

P.P.S. https://www.diffchecker.com/y0I0exOo - the code on the left is 1.1.0 and that doesn't work; the code on the right works.

Huh, so it seems I'm having some trouble running the code - I was on version 1.0.2 previously and updating to 1.2.0 stopped the plugin from working - so I tested 1.1.0 and it isn't working either for me (I'm on Node v6.11.4). So I've literally gone line by line through the diff between 1.0.2 and 1.1.0, starting with 1.0.2 and replacing everything to match your changes - and the plugin stops working for me after this change near the bottom of `index.js`: ```diff - } - flow.push(file); - return next(); + return next(null, file); + } ``` Any ideas what could cause this? P.S. I'm looking at this diff: https://github.com/corneliusio/gulp-once/commit/d37691e2dfa20de4b19af8b2c3480e12fff36ac0 P.P.S. https://www.diffchecker.com/y0I0exOo - the code on the left is 1.1.0 and that doesn't work; the code on the right works.

corneliusio commented

2018-03-05 22:03:25 +00:00

(Migrated from github.com)

That's a real headscratcher. That diff should be completely equivalent.

I've reverted to pushing the file to the stream explicitly and published the change. Give it a shot (I would really quickly first, but in my current setup I have some await/async calls that aren't supported in node@6).

That's a real headscratcher. That diff should be completely equivalent. I've reverted to pushing the file to the stream explicitly and published the change. Give it a shot (I would really quickly first, but in my current setup I have some await/async calls that aren't supported in node@6).

corneliusio commented

2018-03-05 22:13:39 +00:00

(Migrated from github.com)

I just tested both syntaxes on node 6.11, no issues. Might be something else going on with your setup.

Double check you're definitely working with gulp-once@^1.2.0 (yarn outdated or npm outdated). And if you wouldn't mind sending me your gulp task and any specific error you're getting I could take a look and see what might be going on.

I just tested both syntaxes on node 6.11, no issues. Might be something else going on with your setup. Double check you're definitely working with gulp-once@^1.2.0 (`yarn outdated` or `npm outdated`). And if you wouldn't mind sending me your gulp task and any specific error you're getting I could take a look and see what might be going on.

levacic commented

2018-03-05 23:17:33 +00:00

(Migrated from github.com)

I've just tried updating all of my npm dependencies, thinking it could be something like that - though it's still not working, even with gulp-once@1.2.1. The setup is pretty complicated so it would take me a bit longer to come up with a minimal repro example. However, I've done some debugging as below:

The main difference I see between the newer versions and 1.0.2 is that the return is now within the file.isBuffer() block, and nothing happens if that check fails.

I'm passing ./public/images/**/* into gulp.src() which is immediately piped into gulp-once.

I've added console.log(file.path, file.isBuffer()); at the very beginning of the stream._transform function in gulp-once, and the first (and only) output I get when running my task is:

/opt/public/images/admin false

(This is within a Docker container, and /opt is the project root)

However, public/images/admin is a folder, not a file, so I think this is where the plugin just stops any further processing. Any ideas on how to fix this in the plugin, or whether it's me doing something wrong/unexpected with Gulp?

I've just tried updating all of my npm dependencies, thinking it could be something like that - though it's still not working, even with `gulp-once@1.2.1`. The setup is pretty complicated so it would take me a bit longer to come up with a minimal repro example. However, I've done some debugging as below: The main difference I see between the newer versions and `1.0.2` is that the `return` is now within the `file.isBuffer()` block, and nothing happens if that check fails. I'm passing `./public/images/**/*` into `gulp.src()` which is immediately piped into `gulp-once`. I've added `console.log(file.path, file.isBuffer());` at the very beginning of the `stream._transform` function in `gulp-once`, and the first (and only) output I get when running my task is: ``` /opt/public/images/admin false ``` (This is within a Docker container, and `/opt` is the project root) However, `public/images/admin` is a folder, not a file, so I think this is where the plugin just stops any further processing. Any ideas on how to fix this in the plugin, or whether it's me doing something wrong/unexpected with Gulp?

levacic commented

2018-03-05 23:31:06 +00:00

(Migrated from github.com)

It does indeed seem that the problem is with folders being matched by the glob passed to gulp.src(), because when I call gulp.src(source, {nodir: true}) (as per the node-glob options), the plugin works correctly.

Do you think this is something that the plugin should handle without failing?

It does indeed seem that the problem is with folders being matched by the glob passed to `gulp.src()`, because when I call `gulp.src(source, {nodir: true})` (as per the [`node-glob` options](https://github.com/isaacs/node-glob#options)), the plugin works correctly. Do you think this is something that the plugin _should_ handle without failing?

corneliusio commented

2018-03-06 03:00:00 +00:00

(Migrated from github.com)

Ok, that actually makes sense.

tl;dr
Sorry, I tend to over explain. Most recent version should work for you now. Exposition below, ha.

Yeah, gulp can be a little finicky about directories. Since basically all gulp plugins work off the assumption that you are going to pass it data and either transform the data and pass it along or pass the data along unchanged, the gulp stream tends to be pretty transparent with what goes through it. (i.e. If it goes in, regardless if it's a buffer, stream, directory, or null) it comes back out again) No muss no fuss.

However, with a plugin such as this, you are intentionally being restrictive about what goes through the stream. Which, as I discovered today, has resulted in it being a bit tricky to setup tests. At any rate, this plugin has suffered from a lack of robust testing on my part and where I made a critial mistake was only interacting with buffers in the plugin. Unfortunately, since I'm not using subdirectories in any of my sources that pass through this plugin, I never ran into the issue gulp basically failing silently when it hits a directory. Though, I have actually run into this issue with other plugins and just worked around it by making my src glob something like src/**/*.* which kind of, basically only grabs files. I'd never made the connection with my plugin and this case of passing directories doesn't seem to be explicitly addressed anywhere that I can find in gulps documentation.

Ultimately, the fix is rediculously simple. I only needed to make sure I passed all data along untouched if it wasn't a buffer, and only run all the filtering logic on the buffers. Might ultimately handle the issues I had with setting up tests, but we'll see. Hopefully I'll be able to push out a more robust and flexible 2.0 of this plugin with actual tests so I don't run into an issue like this again. Ha.

Ok, that actually makes sense. **tl;dr** Sorry, I tend to over explain. Most recent version should work for you now. Exposition below, ha. Yeah, gulp can be a little finicky about directories. Since basically all gulp plugins work off the assumption that you are going to pass it data and either transform the data and pass it along or pass the data along unchanged, the gulp stream tends to be pretty transparent with what goes through it. (i.e. If it goes in, regardless if it's a buffer, stream, directory, or null) it comes back out again) No muss no fuss. However, with a plugin such as this, you are intentionally being restrictive about what goes through the stream. Which, as I discovered today, has resulted in it being a bit tricky to setup tests. At any rate, this plugin has suffered from a lack of robust testing on my part and where I made a critial mistake was **only** interacting with buffers in the plugin. Unfortunately, since I'm not using subdirectories in any of my sources that pass through this plugin, I never ran into the issue gulp basically failing silently when it hits a directory. Though, I have actually run into this issue with other plugins and just worked around it by making my src glob something like `src/**/*.*` which kind of, basically only grabs files. I'd never made the connection with my plugin and this case of passing directories doesn't seem to be explicitly addressed anywhere that I can find in gulps documentation. Ultimately, the fix is rediculously simple. I only needed to make sure I passed **all** data along untouched if it wasn't a buffer, and only run all the filtering logic on the buffers. Might ultimately handle the issues I had with setting up tests, but we'll see. Hopefully I'll be able to push out a more robust and flexible 2.0 of this plugin with actual tests so I don't run into an issue like this again. Ha.

levacic commented

2018-03-06 07:43:27 +00:00

(Migrated from github.com)

1.2.2 works great now with the context option, appreciate it! I'll close this issue as the original request has now been implemented.

Also, thanks for the in-depth explanation, I'm totally with you on the over-explaining aspect, and thanks for the only plugin that does what we actually need in terms of caching! :)

One thing that we're doing, which feels a bit weird, but I don't know a better way, is piping through gulp-once twice :) We pass the same options both times - once at the beginning, just after gulp.src(), and once at the end, after gulp.dest(). This way, the has is computed first on entry, and if an image is not cached yet (or its hash has changed), it gets optimized - and then its hash is computed again after the optimization, so it gets stored in the cache file and doesn't get processed again on the next run. It's a micro-optimization I guess, but if you have a better suggestion on how to accomplish this, I'd love to hear it.

1.2.2 works great now with the `context` option, appreciate it! I'll close this issue as the original request has now been implemented. Also, thanks for the in-depth explanation, I'm totally with you on the over-explaining aspect, and thanks for the only plugin that does what we actually need in terms of caching! :) One thing that we're doing, which feels a bit weird, but I don't know a better way, is piping through `gulp-once` twice :) We pass the same options both times - once at the beginning, just after `gulp.src()`, and once at the end, after `gulp.dest()`. This way, the has is computed first on entry, and if an image is not cached yet (or its hash has changed), it gets optimized - and then its hash is computed again _after_ the optimization, so it gets stored in the cache file and doesn't get processed again on the next run. It's a micro-optimization I guess, but if you have a better suggestion on how to accomplish this, I'd love to hear it.

corneliusio commented

2018-03-06 15:30:00 +00:00

(Migrated from github.com)

Hmm… I'm not sure I'm following the reasoning on this, in fact, I would think this could cause issues for you guys. So for example…

src/
  - foo.txt

    gulp.src('src/**/*')
        .pipe(once())
        .pipe(transformPlugin())
        .pipe(gulp.dest('dist'))
        .pipe(once());

I run this, and let's say the first call to once gives me this .checksums

Without context:

{
    "foo.txt": "abcd"
}

With context:

{
    "src/foo.txt": "abcd"
}

Now, this would mean that, at this point, if foo.txt was passed back into this gulp task unchanged it would hit this call to once, see the file exists in the checksums, see that the content hash is still "abcd", and not pass the file along to our transformPlugin() call.

But on this first run, foo.txt continues down the stream and hit's transformPlugin() where it's content is changed and then passed along to gulp.dest(). When it hit's the second call to once() we'll end up with one of the two possibilities.

Without context:

{
    "foo.txt": "efgh"
}

With context:

{
    "src/foo.txt": "abcd",
    "dist/foo.txt": "efgh"
}

So now, let's saw we come back and run gulp again with no changes (i.e. src/foo.txt has a hash of "abcd"). In the first case, gulp will hit once the first time and see that the hash it has doesn't match the hash for foo.txt, since this hash now represents the has generated from the transformed file and not the file in it's source state. So it passes the file along, transforms it, and recalculates the hash again. Meaning that the call to once will always pass files along—not preventing any calls to transformPlugin().

In the second case (with context), with no changes made, once will correctly see that the hash for src/foo.txt is still "abcd"—stopping the file there. At which point, the file never makes it to gulp.dest and never does anything with the dist/foo.txt entry. It's just taking up space in our .checksums file.

Now, let's say we do make a change to foo.txt. In the first case, things will work the exact same way, source will not match hash and the file will continue down the stream.

In the second case, the only key checked against will be src/foo.txt. We'll see a new hash since we changed the files and the file will continue down the stream. And at the end, we'll end up with a new hash value for dist/foo.txt. But keeping in mind that this value will never be checked against anything—we either repeat our above process or this one.

Let me know if this is clarifying, or if totally missed the mark on what you were describing.

Hmm… I'm not sure I'm following the reasoning on this, in fact, I would think this could cause issues for you guys. So for example… ``` src/ - foo.txt ``` ```js gulp.src('src/**/*') .pipe(once()) .pipe(transformPlugin()) .pipe(gulp.dest('dist')) .pipe(once()); ``` I run this, and let's say the first call to once gives me this `.checksums` Without context: ```json { "foo.txt": "abcd" } ``` With context: ```json { "src/foo.txt": "abcd" } ``` Now, this would mean that, at this point, if `foo.txt` was passed back into this gulp task unchanged it would hit this call to once, see the file exists in the checksums, see that the content hash is still "abcd", and not pass the file along to our `transformPlugin()` call. But on this first run, `foo.txt` continues down the stream and hit's `transformPlugin()` where it's content is changed and then passed along to `gulp.dest()`. When it hit's the second call to `once()` we'll end up with one of the two possibilities. Without context: ```json { "foo.txt": "efgh" } ``` With context: ```json { "src/foo.txt": "abcd", "dist/foo.txt": "efgh" } ``` So now, let's saw we come back and run gulp again with no changes (i.e. src/foo.txt has a hash of "abcd"). In the first case, gulp will hit once the first time and see that the hash it has doesn't match the hash for `foo.txt`, since this hash now represents the has generated from the transformed file and not the file in it's source state. So it passes the file along, transforms it, and recalculates the hash again. Meaning that the call to once will **always** pass files along—not preventing any calls to `transformPlugin()`. In the second case (with context), with no changes made, once **will** correctly see that the hash for `src/foo.txt` is still "abcd"—stopping the file there. At which point, the file never makes it to `gulp.dest` and never does anything with the `dist/foo.txt` entry. It's just taking up space in our `.checksums` file. Now, let's say we *do* make a change to `foo.txt`. In the first case, things will work the exact same way, source will not match hash and the file will continue down the stream. In the second case, the only key checked against will be `src/foo.txt`. We'll see a new hash since we changed the files and the file will continue down the stream. And at the end, we'll end up with a new hash value for `dist/foo.txt`. But keeping in mind that this value will never be checked against anything—we either repeat our above process or this one. Let me know if this is clarifying, or if totally missed the mark on what you were describing.

levacic commented

2018-03-06 16:04:09 +00:00

(Migrated from github.com)

Kinda in a rush at the moment so no time for a long answer, but one fact I've failed to mention which might help make more sense is that we're editing the files in-place, ie. the source and the destination are always the same for us.

Our actual task is split into several files, some for configuration, some for the logic, but I've tried to create a self-contained Gulp task to show you what I mean (no guarantees this will work):

'use strict';

const gulp = require('gulp');
const once = require('gulp-once');
const imagemin = require('gulp-imagemin');
const pngcrush = require('imagemin-pngcrush');
const mkdirp = require('mkdirp');
const path = require('path');

const source = './public/images/**/*';
const destination = './public/images';
const cacheFile = './.cache/gulpfile.js/gulp-once.json';

gulp.task('optimize-images', function () {
  mkdirp.sync(path.dirname(cacheFile));

  gulp.src(source)
    // Compare the current images' hash values with what has been stored in the
    // `cacheFile`, and pass through any images that haven't been seen before,
    // or that have changed; store their hashes in the `cacheFile`.
    .pipe(once({
      file: cacheFile,
      context: process.cwd(),
    }))
    // Process the images that haven't been processed yet, or have changed.
    .pipe(imagemin(
      [
        pngcrush(),
        imagemin.jpegtran({
          progressive: true,
        }),
      ]
    ))
    // Write the output files.
    .pipe(gulp.dest(destination))
    // Recalculate the checksums for the processed files - we know this isn't
    // meant to be used like this, because `gulp-once` is supposed to be used
    // for either passing files down the stream, or blocking them - but a useful
    // side-effect is that the hash gets recalculated and stored in our
    // `cacheFile`, which on the next run avoids reprocessing an image that has
    // already been processed.
    .pipe(once({
      file: cacheFile,
      context: process.cwd(),
    }));
});

I've tried to explain in code comments what the code is supposed to be doing - does that maybe make more sense, or is this not going to do what we expect it to? Because in practice, as far as we can tell, it does exactly what we think it does.

Kinda in a rush at the moment so no time for a long answer, but one fact I've failed to mention which might help make more sense is that we're editing the files in-place, ie. the source and the destination are always the same for us. Our actual task is split into several files, some for configuration, some for the logic, but I've tried to create a self-contained Gulp task to show you what I mean (no guarantees this will work): ```js 'use strict'; const gulp = require('gulp'); const once = require('gulp-once'); const imagemin = require('gulp-imagemin'); const pngcrush = require('imagemin-pngcrush'); const mkdirp = require('mkdirp'); const path = require('path'); const source = './public/images/**/*'; const destination = './public/images'; const cacheFile = './.cache/gulpfile.js/gulp-once.json'; gulp.task('optimize-images', function () { mkdirp.sync(path.dirname(cacheFile)); gulp.src(source) // Compare the current images' hash values with what has been stored in the // `cacheFile`, and pass through any images that haven't been seen before, // or that have changed; store their hashes in the `cacheFile`. .pipe(once({ file: cacheFile, context: process.cwd(), })) // Process the images that haven't been processed yet, or have changed. .pipe(imagemin( [ pngcrush(), imagemin.jpegtran({ progressive: true, }), ] )) // Write the output files. .pipe(gulp.dest(destination)) // Recalculate the checksums for the processed files - we know this isn't // meant to be used like this, because `gulp-once` is supposed to be used // for either passing files down the stream, or blocking them - but a useful // side-effect is that the hash gets recalculated and stored in our // `cacheFile`, which on the next run avoids reprocessing an image that has // already been processed. .pipe(once({ file: cacheFile, context: process.cwd(), })); }); ``` I've tried to explain in code comments what the code is supposed to be doing - does that maybe make more sense, or is this not going to do what we expect it to? Because in practice, as far as we can tell, it does exactly what we think it does.

corneliusio commented

2018-03-06 16:21:56 +00:00

(Migrated from github.com)

Ahhh, yes. Then that would change everything. In that case what you're doing is correct. While I personally am not a huge fan of destructive transforms (i.e. overwriting source files), there shouldn't be anything wrong with this setup. And, in fact, there wouldn't be any way to get around having two function calls in your pipeline to the same plugin.

A plugin written to be more explicitly used in this way might expose two methods, something like a once.test() for reading and filtering and once.update() to update the stored hash. It just so happens that the call to once() will simply do both regardless.

I'm trying to think through scenarios where this behavior might cause issues, but I can't off the top of my head. If for some reason, you guys come across a situation where it does you can def let me know and I can change that functionality to be controlled more explicitly.

Ahhh, yes. Then that would change everything. In that case what you're doing is correct. While I personally am not a huge fan of destructive transforms (i.e. overwriting source files), there shouldn't be anything wrong with this setup. And, in fact, there wouldn't be any way to get around having two function calls in your pipeline to the same plugin. A plugin written to be more explicitly used in this way might expose two methods, something like a `once.test()` for reading and filtering and `once.update()` to update the stored hash. It just so happens that the call to `once()` will simply do both regardless. I'm trying to think through scenarios where this behavior might cause issues, but I can't off the top of my head. If for some reason, you guys come across a situation where it does you can def let me know and I can change that functionality to be controlled more explicitly.

levacic commented

2018-03-06 21:46:37 +00:00

(Migrated from github.com)

I guess separate methods might make sense for readability and explicitness, but our team is perfectly fine with using it like this.

I think it might cause issues if one would want to pipe some additional stuff after the second call to once(). Right now, the next pipe would receive only the files which were updated (e.g. optimized images in our case) - though if one would want to do something with those, it's something that might even be useful - so I'm not sure whether separating the functionality is required. Maybe something more semantic like once.onlyNewOrChanged() and once.updateCache() might work to cover all those situations in a readable way.

Either way, if we encounter any issues with our current workflow, we'll be sure to let you know :)

As for destructive transforms, I wholeheartedly agree with you on that point in general, and I also prefer storing data in some kind of a source format, and then providing a transformed output separately as needed - an example that comes to mind is when people protect against XSS by escaping HTML before storing it in the database, while the correct course of action is of course to store it as entered, and escape it when outputting (different outputs might require different escaping strategies).

That said, we do make an exception in this specific case for two reasons. The first one is completely pragmatic - we prefer to keep the size of our repos lower, so we don't even commit the images before they're optimized (unless by mistake of course, though we try not to). The second reason sort of builds upon that - in an ideal world we would expect to already have fully optimized images before using them (e.g. a designer performing optimization before providing images to developers) - which implies that we don't really care enough about the non-optimized images to store them in our codebase history. So while the optimization is likely a destructive transformation, for our specific use-case we don't care about the original data at all, thus we're fine with this :)

Thank you for the interesting conversation and for acting so quickly on the original issue - during the updates to our build process we've actually decided to go with your advice regarding tracking the cached hashes in our repos - as it makes perfect sense to do that!

Cheers :)

I guess separate methods might make sense for readability and explicitness, but our team is perfectly fine with using it like this. I think it might cause issues if one would want to pipe some additional stuff _after_ the second call to `once()`. Right now, the next pipe would receive only the files which were updated (e.g. optimized images in our case) - though if one would want to do something with those, it's something that might even be useful - so I'm not sure whether separating the functionality is _required_. Maybe something more semantic like `once.onlyNewOrChanged()` and `once.updateCache()` might work to cover all those situations in a readable way. Either way, if we encounter any issues with our current workflow, we'll be sure to let you know :) As for destructive transforms, I wholeheartedly agree with you on that point in general, and I also prefer storing data in some kind of a _source_ format, and then providing a transformed output separately as needed - an example that comes to mind is when people protect against XSS by escaping HTML before storing it in the database, while the correct course of action is of course to store it as entered, and escape it when outputting (different outputs might require different escaping strategies). That said, we do make an exception in this specific case for two reasons. The first one is completely pragmatic - we prefer to keep the size of our repos lower, so we don't even commit the images before they're optimized (unless by mistake of course, though we try not to). The second reason sort of builds upon that - in an ideal world we would expect to already have fully optimized images before using them (e.g. a designer performing optimization before providing images to developers) - which implies that we don't really care enough about the non-optimized images to store them in our codebase history. So while the optimization is likely a destructive transformation, for our specific use-case we don't care about the original data at all, thus we're fine with this :) Thank you for the interesting conversation and for acting so quickly on the original issue - during the updates to our build process we've actually decided to go with your advice regarding tracking the cached hashes in our repos - as it makes perfect sense to do that! Cheers :)

corneliusio commented

2018-03-06 21:55:30 +00:00

(Migrated from github.com)

For sure! It was good to talk through these things with you and hear how you all were are using the plugin.

And just as an FYI, I was able to get a lot done in the way of testing today and was able to work in both things we've discussed in this thread. If you're interested, I published the 2.0 version a little earlier—biggest change being a default of {context: process.cwd()} but you can also now pass a function to namespace and dynamically build that up. And with the tests setup now, you shouldn't have to worry about updating in the future.

Cheers mate.

For sure! It was good to talk through these things with you and hear how you all were are using the plugin. And just as an FYI, I was able to get a lot done in the way of testing today and was able to work in both things we've discussed in this thread. If you're interested, I published the 2.0 version a little earlier—biggest change being a default of `{context: process.cwd()}` but you can also now pass a function to `namespace` and dynamically build that up. And with the tests setup now, you shouldn't have to worry about updating in the future. Cheers mate.

Rows
Columns

Enable relative paths for cache keys #1