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

"Keep times" option #435

Closed
wants to merge 1 commit into from
Closed

"Keep times" option #435

wants to merge 1 commit into from

Conversation

akx
Copy link
Contributor

@akx akx commented Mar 9, 2020

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

This adds the "keep file times" functionality discussed in #396.

Breaking Changes

This functionality is opt-in, so nothing should break (except maybe if this plugin is used in a web context without a shim for fs, see below).

Additional Info

The code in updateTimes is a little finicky since we need to find an implementation of utimes to use. When run in a web context (or with an output file system we don't support), turning keepTimes on will just result in a bunch of warnings.

According to webpack/webpack@e9c0d06 the default output filesystem in the forthcoming Webpack 5 will just be graceful-fs, which has utimes natively. If the output filesystem object has utimes, we expect it to be a function that smells like fs.utimes(). (This will work for any custom filesystem object too.)

For Webpack 4, however, the default output file system in Webpack 4 is a custom NodeOutputFileSystem which lacks utimes altogether. However, since it is a simple wrapper around fs, we can assume that fs.utimes() works fine for assets output with it.

@jsf-clabot
Copy link

jsf-clabot commented Mar 9, 2020

CLA assistant check
All committers have signed the CLA.

@alexander-akait
Copy link
Member

@akx Do you still need this feature? I can write full roadmap for correct implementation

@akx
Copy link
Contributor Author

akx commented Apr 27, 2020

@akx Do you still need this feature? I can write full roadmap for correct implementation

Sure. This implementation seems to work though, it's just the npm audit snafus keeping it back. What's wrong with the implementation in your opinion?

@alexander-akait
Copy link
Member

@akx sorry, for delay, it should be solved on webpack side:

In the future, it will also allow other plugins modify stats

@akx
Copy link
Contributor Author

akx commented May 26, 2020

@evilebottnawi

So... that means changes to both webpack-sources and webpack core itself, with the uncertainty of whether or not they'll get approved at all to begin with? Also, considering these are API changes, they'd probably be version-major?

If I rebase this PR, would it be approved until stats collection becomes (if it ever will be) implemented in Webpack itself?

@alexander-akait
Copy link
Member

We can't implement it on plugin side, we use compilation.hooks.additionalAssets hook, and on this hook assets do not exist yet on disk, they exit only on compilation.hooks.assetEmitted hook and on it we cannot modify assets, It’s also worth saying that we don’t know in what environment the webpack works, so fs module can't exists (I prepare a PR to use webpack input file-system for fast-glob so CopyWebpackPlugin can work in browsers without any problems). Therefore, this needs to be improved on the side of the webpack itself, and not the plugin.

We already had a discussion about this, and it was decided that the implementation on webpack-sources side it the best solution. Why we still haven’t done it? Because this is a rare request.

No need to be afraid to improve something.

@akx
Copy link
Contributor Author

akx commented Jul 21, 2020

they exit only on compilation.hooks.assetEmitted hook and on it we cannot modify assets

We're only modifying filesystem metadata that Webpack doesn't care about. That sounds like it should be safe.

, It’s also worth saying that we don’t know in what environment the webpack works, so fs module can't exists

The PR is aware of this and does a best effort to keep times: https://github.com/webpack-contrib/copy-webpack-plugin/pull/435/files#diff-2e0f2c5a9d0afaccd39535254a1c9076R1-R20

@alexander-akait
Copy link
Member

This implementation unsafe and potential break webpack-dev-server, solution is above

@akx
Copy link
Contributor Author

akx commented Dec 7, 2020

I guess I'll just close this...

@akx akx closed this Dec 7, 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.

3 participants