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

Add output file timestamps for Gulp 4. #303

Closed
wants to merge 2 commits into from

Conversation

bingnz
Copy link

@bingnz bingnz commented Sep 17, 2018

Fixes #301.
This modification sets the atime and mtime timestamp properties so that Vinyl will set the file times on disk correctly.
This happened by default in earlier versions of Gulp, but is no longer automatic in Gulp 4.

@bingnz bingnz changed the title fix: add output file timestamps for Gulp 4. Add output file timestamps for Gulp 4. Sep 17, 2018
@yocontra
Copy link
Member

Going to hold off on this as gulpjs/vinyl#105 needs to be done to fix the root issue of atime/mtime behavior.

If you're interested in fixing this in gulp 4 you can lend a hand on that ticket.

@bingnz
Copy link
Author

bingnz commented Sep 18, 2018

Thanks @contra. I would like to help out if I can find some time; I've been taking a look at some of the things the Gulp team are doing around the better-stats repo and so on. I don't think I have enough context to know what actually needs to be done to get it across the line but if you have any pointers or even a high-level list of tasks I'm happy to chip away at some and I'm sure other people would too. Things like adding some tests for better-stats perhaps? Trying to integrate it with vinyl?

@yocontra
Copy link
Member

@bingnz Here's a thread about it: gulpjs/vinyl-fs#143

Basically the way I see it working is:

  • better-stats would be created to do exactly that - a better stats object, and it would handle the ctime (mode change) attribute automatically any time it was changed
  • a vinyl instance would handle updating its stats atime (access time) and mtime (modify time) whenever the contents attribute was modified (Buffer.is to compare), or stream was piped to something (would be tricky to determine if the output as actually changed or not, but possible). Would probably want to use a proxy here.
  • vinyl-fs would need to be modified to work with the new stat objects, and support setting ctime/mtime/atime properly on write in dest.

@WraithKenny
Copy link

It's been a while, and although I think many of us would like to help with the upstream, but find it all beyond us, I think it's time to fix this here instead.

gulpjs/gulp#2193 (comment) Points out that this is indeed the correct "Gulp way" to do this.

dlmanning/gulp-sass#763 (comment) gulp-sass has decided to merged this same solution.

(A slight difference is also setting ctime in addition ala file.stat.atime = file.stat.mtime = file.stat.ctime = new Date(); but otherwise, I think this patch is good to go.)

@yocontra
Copy link
Member

Sorry but this really needs to be done outside of the plugin (gulpjs/vinyl#105). I'm personally going to take ownership of fixing that ticket in core, this issue has persisted for a lot longer than anticipated.

@WraithKenny I think you misinterpreted his comment, that isn't the gulp way - I'm the original author of gulp and can confirm definitively that it needs to be fixed in core. Every plugin author should not need to re-implement this same behavior - making life easier for plugin authors has always been a huge priority for me.

@yocontra yocontra closed this Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gulp 4 and mtime on output css file
3 participants