Skip to content

copyFiles doesn't works for files required in js #490

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
etshy opened this issue Jan 12, 2019 · 13 comments · Fixed by #518
Closed

copyFiles doesn't works for files required in js #490

etshy opened this issue Jan 12, 2019 · 13 comments · Fixed by #518
Labels

Comments

@etshy
Copy link

etshy commented Jan 12, 2019

I got a weird behaviour with copyFiles.

I know in the docs, the copyFiles is used fore images in templates, but, in my case, and I guess in many cases, I have images in CSS and in templates.

For example with this webpack config

.copyFiles({
        from: './assets/img',
        // optional target path, relative to the output dir
        //to: 'images/[path][name].[ext]',

        // if versioning is enabled, add the file hash too
        to: 'img/[path][name].[hash:8].[ext]',

        // only copy files matching this pattern
        pattern: /\.(png|jpg|jpeg|gif|ico|svg|webp)$/
    })

In this case, the images are copied into 2 folders.
The images from templates follow the copyFiles rules and end up in public/build/img/[path]
The images from CSS/JS are copied into public/build/images/*.jpg|png|etc.

and if I have the same images used in template and in CSS/JS, the images will be duplicated, one on each build sub folder.

It could be good if all images followed the same rules, no ?
Or I missed something in the docs ?

Edit : Sorry I misinterpreted something.
It seems copyFiles copy all files, not only those used in templates.
But for some reasons I also have images copied into an images folder and I don't understand what is causing this.
The images copied into the images folder are only images used in CSS (used in my CSS "framework", fomantic-ui).

@weaverryan
Copy link
Member

Hi there!

Yea, there is some inconsistency internally. When you reference an image in css, we move that for you always into images. Then there is this completely separate functionality that copies files - and the destination is wherever you want. So, if you reference an image from css and also copy that file, it will end up in 2 destinations.

We could potentially unify this somehow, maybe, but I’m not sure.

@etshy
Copy link
Author

etshy commented Jan 12, 2019

Okay so that's normal for now, but it's kinda weird, imo.

I don't know if it's possible, but is it possible to detect if the copyfiles method is used ?

And if it's used then use the to and pattern options to move the files with the same rules.

@Lyrkan
Copy link
Collaborator

Lyrkan commented Jan 14, 2019

Hi @etshy,

I don't know if it's possible, but is it possible to detect if the copyfiles method is used ?
And if it's used then use the to and pattern options to move the files with the same rules.

I don't think that would be a good idea at all.

Let's say you have the following config:

Encore.copyFiles({
    from: './assets',
    to: 'assets/[path][name].[hash:8].[ext]',
    pattern: /\.(png|csv)$/
});

What should Encore do:

  • for a .png file from ./assets?
  • for a .png file from ./foo imported in a .js or .css file?
  • for a .csv file from ./assets?
  • for a .jpg file from ./assets imported in a .js or .css file?

I mean... we could decide what should be the result of each one of these cases but if that isn't obvious for us right now it won't be obvious for the end-user either.

And this is probably the easiest thing to deal with because then we would, for instance, have to handle multiple calls to copyFiles with intersecting pattern and from options.

In my opinion copyFiles shouldn't have any impact on how require() (or url(...) in CSS files) works.
If you really need to avoid duplicate files you should already be able to do it by using the same path (including the hash) in configureFilenames() and copyFiles().

@etshy
Copy link
Author

etshy commented Jan 14, 2019

I see your point there.

I tried to have configureFilenames() and copyFiles() byt the [path] part on both doesn't work the same way.
If I'm not mistaken, The [path] in copyFiles() is the path inside the from options
And the [path] in configureFilenames() is the 'full path' from the project dir (or something like that).

Is there a way, currently, to have the same "behaviour" ?

Here is what I tried, if I have the following configureFilenames(), the images goes to public/build/img/assets/img/... (my images are in ./assets/img/...

.configureFilenames({
        images: 'img/[path][name].[hash:8].[ext]'   
    })

while my images from copyfiles() goes to public/build/img/... with the exact same path.

If it's not possible, Maybe make a way to create custom variable to use in the path like the following.

.configureFilenames({
        images: 'img/[pathAssets][name].[hash:8].[ext]'   
    },
    variables: {
        'pathAssets': './assets/img'
    }
)

It will recreate the path inside './assets/img'.

It would allow to have the files in same folder.

Just a quick idea I just had though.

@Lyrkan
Copy link
Collaborator

Lyrkan commented Jan 14, 2019

Hmm... you may be right, the context is not the same in copyFiles (path given in from) and configureFilenames (project root).

I don't think introducing a custom placeholder would be a good thing since the other ones are kinda standards (defined by the file-loader: https://github.com/webpack-contrib/file-loader#placeholders).

It may still be possible to get a config in which files override each others but with some trade-offs (such as not using [path] - which is the default behavior for the file-loader - or not including subdirectories in copyFiles).

Something we may be able to do however is to allow people to set the the context when calling copyFiles().

It could look like that:

Encore
  .configureFilenames({ images: 'img/[path][name].[hash:8].[ext]' })
  .copyFiles({
    context: './',
    from: './any/directory',
    to: 'img/[path][name].[hash:8].[ext]'
    pattern: /.png$/
  })
;

Or another solution, simply add a flag to use the same context than the one used in other loaders:

Encore
  .configureFilenames({ images: 'img/[path][name].[hash:8].[ext]' })
  .copyFiles({
    useDefaultContext: true,
    from: './any/directory',
    to: 'img/[path][name].[hash:8].[ext]'
    pattern: /.png$/
  })
;

@etshy
Copy link
Author

etshy commented Jan 14, 2019

Adding a context could be good, but the configureFilenames()'s [path] will still copy the whole path ( = assets/img/..., no ?

And so, the whole path will be copied into the public/build/... folder.

The best thing would be to choose the root folder for the context too, I think, but I guess it's not really possible with the webpack placeholder ?

@Lyrkan
Copy link
Collaborator

Lyrkan commented Jan 14, 2019

You can actually already specify the context for images/fonts... but that's not something obvious at all :)

By default the file-loader is used... which you can't really configure (we should probably add a configureFileLoader method for that, WDYT @weaverryan ?).

However, there is an alternative to that loader: the url-loader. It can be used to embed files directly using a data URL if they don't exceed a given filesize... or use the file-loader with the same configuration if they do.

And the thing is: the url-loader can be fully configured using configureUrlLoader, so you can do something like this:

Encore.configureUrlLoader({
  images: {
    limit: 1, // Will use the file-loader if over a byte
    context: '...'
  }
});

Now if you choose to change the context I recommend you to read the following comment before to make sure you understand what the resulting paths will be (especially if you import things from node_modules): #103 (comment)

These "ugly paths" were the reason we choose to not include the [path] placeholder by default.

@etshy
Copy link
Author

etshy commented Jan 14, 2019

Thanks I'll read more about it.

But, from my quick view of the github issues, and urlLoader github page I didn't see any context option (fallback, mimetype and limit only).

I'll make some test when I can.

Edit : After few tests context option doesn't seem to change anything.

@Lyrkan
Copy link
Collaborator

Lyrkan commented Jan 15, 2019

@etshy The context option is not documented as one of the url-loader options because it is not handled by that loader itself. However it is also noted that: The fallback loader will receive the same configuration options as url-loader., so it should be passed to the file-loader which is the default fallback.

I did a quick test and it seems to work on my side.

Given this project structure (the index.js file only does a require('../assets/images/350x150.png')):

.
├── assets
│   └── images
│       └── 350x150.png
├── src
│   └── index.js
└── webpack.config.js

And the following webpack.config.js:

// webpack.config.js
const Encore = require('@symfony/webpack-encore');

Encore
  .setOutputPath('build/')
  .setPublicPath('/')
  .cleanupOutputBeforeBuild()
  .enableSingleRuntimeChunk()
  .addEntry('main', './src/index.js')
  .configureFilenames({
    images: './images/[path][name].[hash:8].[ext]'
  })
;

module.exports = Encore.getWebpackConfig();

I get this build structure:

build
├── entrypoints.json
├── images
│   └── assets
│       └── images
│           └── 350x150.18274ad3.png
├── main.js
├── manifest.json
└── runtime.js

Now, if I add this code before module.exports:

Encore.configureUrlLoader({
  images: {
    limit: 1,
    context: './assets/images'
  }
});

The build directory doesn't have the extra assets/images anymore:

build
├── entrypoints.json
├── images
│   └── 350x150.18274ad3.png
├── main.js
├── manifest.json
└── runtime.js

@etshy
Copy link
Author

etshy commented Jan 16, 2019

It worked yeah, thanks a lot

But for the images/fonts in my CSS framework it generated a weaird path.

I have this structure

assets
    - semantic
        - dist
            ....

And I get this build

build
    - img
        - "_"
            - semantic
                - dist
                    - ....

for the following webpack config

    .copyFiles({
        from: './assets/img',
        // optional target path, relative to the output dir
        //to: 'images/[path][name].[ext]',

        // if versioning is enabled, add the file hash too
        to: 'img/[path][name].[hash:8].[ext]',

        // only copy files matching this pattern
        pattern: /\.(png|jpg|jpeg|gif|ico|svg|webp)$/
    })

    .configureFilenames({
        images: 'img/[path][name].[hash:8].[ext]'
    })

    .configureUrlLoader({
        images: {
            limit: 1,
            context: './assets/img'
        }
    })

the _ part is a bit weird.
except that it's perfect.

Thanks again.

@Lyrkan
Copy link
Collaborator

Lyrkan commented Jan 16, 2019

But for the images/fonts in my CSS framework it generated a weaird path.

That's what I was talking about at the end of one of my previous comment and why I recommended you to read #103 (comment).

Underscores are used in [path] to basically replace ".." when you try to import something that is outside of your context.

@etshy
Copy link
Author

etshy commented Jan 16, 2019

Yep I missed that part in your comment.

Well, the build is working anyway so that's fine, but still, I think this is weird to have this kind of generation.

Thanks for all your help and advices.

@Lyrkan Lyrkan added the HasPR label Feb 10, 2019
weaverryan added a commit that referenced this issue Mar 1, 2019
…erryan)

This PR was merged into the master branch.

Discussion
----------

Allow to set a custom context in copyFiles

The current behavior of `copyFiles()` is to use the value of the `from` option as a context when copying files.

This means that when doing the following:

```js
Encore.copyFiles({
  from: './assets/images',
  to: '[path][name].[ext]',
  includeSubdirectories: false,
});
```

Will end-up putting all the files from `./assets/images` directly at the root of the output directory.

Sometimes this is not the desired behavior (see #490 (comment)) which is why this PRs adds a `context` option to `copyFiles()` entries (closes #490).

For instance:

```js
Encore.copyFiles({
  from: './assets/images',
  to: '[path][name].[ext]',
  includeSubdirectories: false,
  context: './',
});
```

Will put all the copied files inside of an `./assets/images` folder in the output directory.

Commits
-------

73b8be5 a bit more docs
487d6ef Allow to set a custom context in copyFiles
@codegain
Copy link

@Lyrkan How would you configure that in the current version of encore? I'm stuck with the same issue and configureUrlLoader is not available anymore.

For

assets
- images
  - icons
  - graphics

I tried this config:

    .copyFiles({
        from: './assets/images',
        to: 'images/[path][name].[hash:8].[ext]',
    })
    .configureImageRule({
        filename: './images/[path][name].[hash:8].[ext]',
    })

but this results in copied files in build/images/icons + build/images/graphics and all referenced assets in JS/CSS in build/images/assets/images/icons + build/images/assets/images/graphics

Sadly there is no context option for configureImageRule. I would really like all my assets to be deduplicated and in build/images/[path].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants