refactory copy method, fix #34

[tinchoz49]

Sorry, it was an issue in the new tests but fixed now. everthing is working fine.

About the issue #34 this is what i did...

tested with this idea of folders:

dist/
src/
  images/
    i.jpg
  page/
    index/
      images/
        a.jpg
      index.css     # this file has url('images/a.jpg') and url('../../images/i.jpg')
  main.css          # this file has url('images/i.jpg')

postcss set with

from: 'src/**/*.css' (example using gulp)
to: 'dist/main.css'

Using postcss without an assetsPath and a useHash=false:

the result is going to be basically the same as the src folder

dist/
  images/
    i.jpg
  page/
    index/
      images/
        a.jpg
      index.css   # this file has url('images/a.jpg') and url('../../images/i.jpg')
  main.css        # this file has url('images/i.jpg')

Using postcss with assetsPath ('assets') and a useHash=false:

dist/
  assets/
    images/
      i.jpg
    page/
      index/
        images/
          a.jpg
  page/
    index/
      index.css   # this file has url('../../assets/page/index/images/a.jpg') and url('../../assets/images/i.jpg')
  main.css        # this file has url('assets/images/i.jpg')

Using postcss with assetsPath ('assets') and a useHash=true:

dist/
  assets/
    hashunique(a).jpg
    hashunique(i).jpg
  page/
    index/
      index.css   # this file has url('../../assets/hashunique(a).jpg') and url('../../assets/hashunique(i).jpg')
  main.css        # this file has url('assets/hashunique(i).jpg')

[MoOx]

This looks good. Since modifications seems a bit complicated, and according to the fact that I do not use this feature, can some others might do a code review as well ? poke @zoubin @lexich

[zoubin]

OK, I'll do it

[zoubin]

@tinchoz49 Thanks for your work.

I've read the code carefully, and run some examples to see the effect. I'm afraid the implemented feature is a little different from what I was expecting in #34 . Perhaps I should have given a more detailed explanation about it.

In /path/to/src/page/index/index.css we have:

.sp-icon {
  background-image: url(i/sp-icon.png);
}

And the sprite lies in /path/to/src/index/i.

I was hoping to specify an absolute assetsPath like:

postcss(url({ assetsPath: '/path/to/build/sp' })
  .process(contents, {
    from: '/path/to/src/page/index/index.css',
    to : '/path/to/build/page/index/index.css'
  })

And the expected transformed css contents should be:

.sp-icon {
  background-image: url(../../sp/sp-icon.png);
}

As I put all my sprites together in the same directory sp, so I need to specify the absolute assets path. Perhaps it should be called assetsDestination.

I'm afraid the absolute path will be included in the final css in this pr.

However, since we have three ways to implement the copy now, perhaps it is worth further discussion to choose one?

@MoOx @lexich Which case do you deal with more often?

[tinchoz49]

Hi @zoubin ! first, it's nothing i enjoy work with postcss and the idea behind, so i want to help :)

I create a repo to show you my idea of how use postcss-url with the copy method, tell me what do you think about it ;)

https://github.com/tinchoz49/test-postcss-copy

[tinchoz49]

Hi @zoubin! how are you?, did you see the example? the idea of the assetsPath is define a relative path from you project, i don't understand what is the scenary that you need to work with absolutes path.

[zoubin]

@tinchoz49 Sorry for my delay in responding. I have tried the example.

In the example, gulp --with-assets-path gave the dist directory:

dist/
├── assets
│   ├── images
│   │   └── a.jpg
│   └── page
│       ├── images
│       │   └── b.jpg
│       └── index
│           └── images
│               └── c.png
├── main.css
└── page
    └── index
        └── index.css

What I want is something like:

dist/
├── assets
│   ├── a.jpg
│   ├── b.jpg
│   └── c.png
├── main.css
└── page
    └── index
        └── index.css

main.css:

body {
    background: url('assets/images/a.jpg');
}

index.css:

body {
    background: url('../../assets/c.png')
}

div {
    background: url('../../assets/b.jpg')
}

span {
    background: url('../../assets/a.jpg')
}

So, I do not mean to specify the url by assetsPath, but to specify where all pics go, one directory for all pics.

I've got an idea. How about assetsPath being a callback as well as a string?

Things get busy right now. I have to go off. I will give an example in a few hours.

[tinchoz49]

ahhhhh now i understand! you can do that with the hash option ;) try the example gulp --with-assets-path-and-hash

The problem here is that, in the situation that you explained to me, you can not use the real name for your images and share the same assetsPath, because you are going to have a name's conflict with your assets. For example imagine that you have two pages folder (pageOne and pageTwo) and you have an image called little-cat.jpg in each one, but in the pageOne that image is a white funny cat and the pageTwo is a red funny cat, with your idea the result is going to be

dist/
  assets/
    little-cat.jpg (but is the white or the red cat?)

so, the idea of using a hash name is that the new name is created based on the content of the file and not in the real name. With this functionality we avoid the name's conflict of our assets.

[MoOx]

Sorry if I didn't handle this PR, but I am not using this feature and the changes seems a bit complicated, not sure we should handle that... Can you rebase + add some doc in the README (in order to easily understand the benefit/changes here)?

[tinchoz49]

I understand @MoOx, really. Maybe is better if i do a separate plugin with this feature and close this pull request, what do you think? I don't know how explain the benefits of the copy method. In the example that i made the last month you can see how it works.

In my case, i only use the the copy method with the hash option like the file-loader + url-loader of webpack. With this option i can create a build folder with all necesary files (js, css, assets) to compile my crosswalk mobile apps.

But for example, i don't like the option of keep the real structure of the assets in the final (to) path in the copy process. I don't see the benefits in that. The copy method without that option would simplify a lot the code and the process.

[MoOx]

I have to admit that maybe this module is becoming too big.

[TrySound]

@tinchoz49 fs.exists and fs.existsSync was deprecated. Recommend to use this one path-exists

[MoOx]

The best thing to do is to not use a fs test at all, and directly try to open, but it's kind of tricky sometimes...

[TrySound]

Agree

[tinchoz49]

@MoOx i agree with you and i think is not follow the postcss way, the copy method must be a separated module. I'm going to start a new repo for a plugin called postcss-copy or if the community has a better name to share, is welcome :+1:

@TrySound i'm going to change that in the new plugin. In the begin i used read directly the file with a try catch but the v8 can't optimize functions with try catch inside but did not know the existence of path-exists. thanks!

[MoOx]

@tinchoz49 Did you take a look to postcss-assets ?

[tinchoz49]

@MoOx yes! it's a nice plugin, but not follow my idea. I want to something really simple like webpack-file-loader