mrsum / webpack-svgstore-plugin

Simple svg-sprite creating with webpack
https://www.npmjs.com/package/webpack-svgstore-plugin
200 stars 90 forks source link

SvgStore creates empty temp directory #51

Closed tb closed 8 years ago

tb commented 8 years ago

I have setup similar to https://github.com/cngroupdk/hands-on-flexbox-svg/blob/master/webpack.config.js#L25

new SvgStore(path.join(__dirname, 'svg-source', '**/*.svg'), path.join('sprites'), {
  name: '[hash].sprite.svg',
  chunk: 'app',
  prefix: 'icon-'
})

in my case it is:

new SvgStore('bower_components/uirepo/frontend/icons/*.svg', path.join('icons'), {
  name: '[hash].icons.svg',
  chunk: 'app',
  prefix: ''
})

In both cases, I get empty folder created (sprites/ and icons/).

I know it could be avoided, by using:

-    new SvgStore(path.join(__dirname, 'svg-source', '**/*.svg'), path.join('sprites'), {
+    new SvgStore(path.join(__dirname, 'svg-source', '**/*.svg'), path.join('svg-source'), {

But with bower_components this is not possible.

How can we improve it so webpack-svgstore-plugin is not creating that empty directory?

mrsum commented 8 years ago

Hi, SvgStore has apply 3 arguments on constructor function. first - source folder, second param is output folder and third param is settings. If output folder has not been detected, plugin will create it for sprite.

Can you explain your problem a little bit more?

tb commented 8 years ago

Ok, I will try to explain it better.

SvgStore has apply 3 arguments on constructor function, lets call them: source, dest and options.

When source includes different name like "svg-source":

new SvgStore(path.join(__dirname, 'svg-source', '**/*.svg'), path.join('sprites'), {

then there is side effect of creating empty dir named "sprites" in root of the project.

I think this is because SvgStore creates temp file inside dest folder in root of project, and then webpack moves the sprite file to dist (see https://github.com/cngroupdk/hands-on-flexbox-svg/blob/master/webpack.config.js#L11) leaving empty directory with webpack setup like:

output:{
  path:__dirname + '/dist',
  filename: "bundle.js"
},

I think its easy to reproduce: 1) add output path:__dirname + '/dist', 2) use different soruce dir and dest dir name

mrsum commented 8 years ago

Yes you are right!

It really happen, can U say what kind of behaviour you are waiting? I try to fix it.

mrsum commented 8 years ago

Hello again.

Can you look up to this branch https://github.com/mrsum/webpack-svgstore-plugin/tree/test/issue-51 Whats change.

I want to offer convention. If you are will use absolute path into svgstore config output path, its will work, but assync runner code will not apply to chunk. and chunk param will be ignored.

And i prepare test case

var runAbsolutePathsExample = function(done) {
  var instance = new Plugin(path.join(__dirname, '..', 'svg-source', '**/*.svg'), path.join(__dirname, '..', 'sprites'), {
    name: 'issue51.[hash].sprite.svg',
    chunk: false, // if chunk is equal to false, 
    prefix: 'icon-',
    svgoOptions: {}
  });

  // @see https://github.com/mrsum/webpack-svgstore-plugin/issues/51
  // replace plugin config
  config.plugins = [instance];

  webpack(config, function(log) {
    done();
  });
};
tb commented 8 years ago

I tested on https://github.com/tb/hands-on-flexbox-svg/tree/test-issue-51

"sprites" folder in root of project is not created anymore, which is :+1:

Thank you!

mrsum commented 8 years ago

@tb Well done. So, i hope tomorrow will be available next release. Stay tune.

lgordey commented 8 years ago

@tb Hi! Try 2.1.3 plugin version

tb commented 8 years ago

I tried the 2.1.3.

With setup:

    new SvgStore(
      path.join(__dirname, 'src/icons/*.svg'),
      path.join(__dirname, 'dist/icons'),
      {
        name: '[hash].icons.svg',
        chunk: 'app',
        prefix: '',
        svgoOptions: {
          plugins: [
            {removeStyleElement: true}
          ]
        }
      })

I see following: 1) in src/icons ".svg" directory is created 2) in dist/Users/tb/projects/project/dist/icons/1234abc.icons.svg is created 3) in dist/icons I have: .svg <- directory 1234abc.icons.svg icon1.svg icon2.svg ...

I expected only dist/icons/1234abc.icons.svg file to be created.

tb commented 8 years ago

I have updated test repo, its possible that "*.svg" folder is left over from previous version issues.

The dist/Users issue is reporduced through. See https://github.com/tb/hands-on-flexbox-svg/tree/test-issue-51/dist

lgordey commented 8 years ago

Ok, give me some time to fix it.

tb commented 8 years ago

Ok, sure.

Another thing is the url in bundle:

document.addEventListener('DOMContentLoaded', svgXHR('/Users/tb/projects/hands-on-flexbox-svg/dist/icons/3326e34bbdcd07bc39016fc8d567e3a6.icons.svg') https://github.com/tb/hands-on-flexbox-svg/blob/test-issue-51/dist/bundle.js#L35

I think it is related to the Users folder issue.

lgordey commented 8 years ago

@tb Hello again. Could you explain to me please: why do you need to specify an absolute path in dest? I'm trying to understand all the cases that can be.

In your situation you don't need to use __dirname in output path option. And what you expect to see in the AJAX, if sprite will not be in project path?

I understand, that source SVGs can be located anywhere, but dest sprite should be in project dest. Or with baseUrl help.

tb commented 8 years ago

@lgordey I thought its required based on README.md example https://github.com/mrsum/webpack-svgstore-plugin#usage

I made branch without dirname, see: https://github.com/tb/hands-on-flexbox-svg/blob/test-issue-51-no-dirname/dist/bundle.js#L35 It still has the same issue of writing my local path to DOMContentLoaded while it should be /icons/[hash].icons.svg

Also it creates dist/dist/icons/[hash].icons.svg - while I wanted file to be just dist/icons/[hash].icons.svg see https://github.com/tb/hands-on-flexbox-svg/tree/test-issue-51-no-__dirname/dist/dist/icons

lgordey commented 8 years ago

@tb Please, try fixed plugin version from branch - https://github.com/lgordey/webpack-svgstore-plugin/tree/feature/fix-paths-bug

lgordey commented 8 years ago

@tb Try 2.1.4 version

oller commented 8 years ago

I suspect this is still a problem with 2.1.4

I've the same problem as @tb, my app has the following structure.

| - node_modules
| - app (source files)
| - dist (prod build)

with the following config:

        new SvgStorePlugin(path.resolve(config.root, "../core/symbols/**/*.svg"), "symbols", {
            name: "[hash].sprite.svg",
            chunk: "main",
            prefix: "symbol-",
            svgoOptions: {
                // options for svgo
                plugins: [
                    { removeTitle: true }
                ]
            }
        })

A symbols directory is created on the root of the project, it is then copied into dist correctly by webpack, however the temp directory remains on the root also.

If I try and fix it with the destination argument being "dist/symbols" then that stops the temp folder being created on the root (as it's moving it into /dist), but then the symbol files end up in dist/dist/symbols

lgordey commented 8 years ago

@oller Thanks for your feedback!

Can you make a little demo to reproduce this error?

oller commented 8 years ago

Aha, I've found a tidier workaround.

I'd rather defer to webpack on where the output files should be, rather than specifying them again, so i'm now passing an empty string into the plugin (for output dir), and using a common pattern used with other loaders to use a file path in the name value to create a subdirectory.

This gets around all the problems and binds everything back to webpack's output.path

        new SvgStorePlugin(path.resolve(config.root, "../core/symbols/**/*.svg"), "", {
            name: "symbols/[hash].sprite.svg",
            chunk: "main",
            prefix: "symbol-",
            svgoOptions: {
                // options for svgo
                plugins: [
                    { removeTitle: true }
                ]
            }
        })

This puts the files in the correct place and removes the incorrect creation of that output folder in the root.

lgordey commented 8 years ago

@oller I think we can try to find the real reason and make it cleaner.

In @tb's demo I saw:

output:{
  path:__dirname + '/dist',
},
new SvgStore(
  path.join(__dirname, 'src/icons/*.svg'),
  'dist/icons',

So that's why he had dist/dist path and it can be easily fixed with config.

oller commented 8 years ago

@lgordey, unfortunately, even with that corrected config - the problem with the temp created folder on the root is still an issue I'm afraid. (this is using the config pasted at : https://github.com/mrsum/webpack-svgstore-plugin/issues/51#issuecomment-205257285 )

I'd have thought the cleanest solution would be to defer as much to webpack's config settings.

In anycase, adding any subdirs into the name value as often cited in other loader examples, seems to get around this, so I'm happy to employ that.

Very handy plugin - thanks again for all your work on it!

lgordey commented 8 years ago

@oller, well, as you wish :) Thanks for your feedback!