Skip to content

[Enhancement] Switch WebpackManifestPlugin for remote package and apply remove hash hack for CopyWebpackPlugin files #1092

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

Closed
wants to merge 3 commits into from

Conversation

shadowc
Copy link

@shadowc shadowc commented Feb 14, 2022

closes #1089

  • Removed fork of WebpackManifestPlugin in favor for the maintained remote repo.
  • Applied a global fix for files coming from CopyWebpackPlugin where hashes would get copied to the manifest "key".

Missing:

  • Add a few tests

map: (file) => {
if (webpackConfig.useVersioning) {
// Remove hash in manifest key
file.name = file.name.replace(/(\.[a-f0-9]{8})(\..*)$/, '$2');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @shadowc,

I'm not sure it would always work... IIRC the hard thing to handle here is that the user has control on the naming, so you can't really assume that:

  • hashes will be 8 characters long (users could chose a different length)
  • hashes will only be composed of hexadecimal characters (hash function can be changed as well)
  • hashes will be at that exact position (eg. [contenthash].[name].js)
  • original filenames won't contain something that looks like a hash (eg. foo.aaaaaaaa.d1fc2a.txt would become foo.d1fc2a.txt)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the heads up! We do have the info at that point of what format the user choose for the to file name, do we? Also, there is a default format [name].[contenthash].[ext] that encore uses?

I'll investigate this further and put against several scenarios to double check!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To answer my own question, I believe we don't have all this information.

I will think of ways to cache regexp patterns for each CopyWebpackPlugin entry together with the position of the hash, then use this to remove such hash from the entry in this function. I believe this could work.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, what I need to cache is the name of the original entry file (not the to field) and just retrieve that somehow.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a simplified version of how this should work:

static_file_mapping = {};
config = {
  plugins: [
    new CopyPlugin([{
            from: {
                glob: 'static/**/*.@(png|svg|jpg|gif|ico)',
                dot: false,
                noext: false,
            },
            to: '[contenthash].[ext]',
            // flatten: true
            transformPath(targetPath, absosutePath) {
                relative_path = path.relative(
                    path.resolve(root_dir),
                    absosutePath)
                static_file_mapping[relative_path] = targetPath;
                return targetPath;
            }
    }]),
    new ManifestPlugin({
      assets: static_file_mapping,
    })
  ]
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that helps here is the PR in which I initially tried to implement copyFiles() using the WebpackManifestPlugin: #221

As you will be able to read in the comments I couldn't make it work at that time because the CopyWebpackPlugin didn't expose original filenames. It looks like some work as been done since then: webpack-contrib/copy-webpack-plugin#309 (comment)

Not sure if that can be used without modifying the WebpackManifestPlugin internals though.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I'll look into this and come up with a better solution!

@shadowc
Copy link
Author

shadowc commented Mar 1, 2022

After a lot of poking around and investigating, I'm putting this PR on hold for a while.

Using CopyWebpackPlugin (which I thought it's what was being used in encore) is probably advised instead of FileLoader but out of the scope of this PR.

Switching to WebpackManifestPlugin (instead of the forked version) is still a possibility, but since we have no hold on the before -> after process of filename transformation, we might need to do something special in FileLoader to generate regexps based on the user configuration, so we can create a map something like this:

const copyFilePathMap =  {
    'original/file/name.js': /^original/file/name.([ah09]{8}).js$/,
};

Where regexps are generated dynamically according to what the user provided in terms of file name format.

This doesn't seem simple at all since the user could strip the file extension, change its name completely, etc...

EDIT: We would then use this hashmap of regexps to check on the Manifest plugin map function, which of the files are being generated with hashes (where they should not be) and fall them back to their original names.

@weaverryan
Copy link
Member

This looks like a dead end for now, but thanks for checking into it!

@weaverryan weaverryan closed this Mar 16, 2022
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.

Improve CopyWebpackPlugin with WebpackManifestPlugin workflow
3 participants