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

vfs.dest(outfolder, { base.. }) ignores outfolder? #141

Closed
iliakan opened this issue Jan 10, 2016 · 10 comments
Closed

vfs.dest(outfolder, { base.. }) ignores outfolder? #141

iliakan opened this issue Jan 10, 2016 · 10 comments

Comments

@iliakan
Copy link

iliakan commented Jan 10, 2016

When dest has a base option, it ignores outfolder, right?
Looks like that from the code.

If yes, then maybe let people specify empty outfolder?

@phated
Copy link
Member

phated commented Jan 12, 2016

can you supply an example of what you are talking about and what your suggestion is?

@iliakan
Copy link
Author

iliakan commented Jan 12, 2016

Well, the example is:

vfs.dest('dst', {
  base: 'somewhere'
})

Here 'dst' is ignored totally, the output folder is inferred from base option.
But still dst must not be empty.

Actually, I'm a little bit confused about the use case of it. If one wants to use a function to calculate a folder, can use:

vfs.dest(function(file) { 
  ...
});

@phated
Copy link
Member

phated commented Jan 15, 2016

I think the base option on dest() is a hold over from not accepting a function. Maybe we can remove it in the 3.0 release. @contra what do you think?

@yocontra
Copy link
Member

@phated I think the fix here is to make the base option work correctly not to remove it. Also open to removing it though if it isn't providing any value

@phated
Copy link
Member

phated commented Mar 28, 2016

@contra what is the correct way it should operate if you are using a function for dest?

@phated
Copy link
Member

phated commented Apr 1, 2016

@iliakan @contra do you think the correct behavior is:

vfs.dest('./test', {
  base: '/my/base'
});
// Writes files at /my/base/test

vfs.dest('/my/path', {
  base: '/other/base'
});
// Writes files at /my/path

This would essentially resolve the dest path from the base path which allows handling of relative paths correctly. Thoughts?

@phated
Copy link
Member

phated commented Apr 2, 2016

Maybe this should just be path.resolve(options.cwd, options.base, outFolder) so any relative paths build up to the full path.

@tmcgee123
Copy link
Contributor

@phated should this change break tests?

I've developed a test branch with what you commented on above, is this what you were thinking?

https://github.com/gulpjs/vinyl-fs/compare/master...tmcgee123:bugfix/dest-outfolder-141?expand=1

@phated
Copy link
Member

phated commented Apr 27, 2016

@tmcgee123 I don't think my last comment was correct. I'm not sure what the "correct" behavior would be. Any thoughts? @contra @iliakan

@phated
Copy link
Member

phated commented May 6, 2016

Thinking through this some more with @stevelacy, the only example I can come up with where the base option on .dest() would be used would be with liberal globs in .src() like:

// src = './client/**/*.sass'
vfs.dest('./dest/styles/');
// => /home/user/project/dest/styles/sass/file.sass
// => /home/user/project/dest/styles/sass/partials/other-file.sass

// src = './client/**/*.css'
vfs.dest('./dest/styles/', {
  base: './sass'
});
// => /home/user/project/dest/styles/file.css
// => /home/user/project/dest/styles/partials/other-file.css

However, it seems that this can be solved by using the base option in the .src() method instead of the .dest() method. I think this option can be removed.

@phated phated closed this as completed in b7a9362 Jun 28, 2016
phated added a commit that referenced this issue Nov 27, 2017
phated added a commit that referenced this issue Nov 27, 2017
phated added a commit that referenced this issue Nov 27, 2017
phated added a commit that referenced this issue Nov 27, 2017
phated added a commit that referenced this issue Nov 27, 2017
phated added a commit that referenced this issue Nov 27, 2017
phated added a commit that referenced this issue Nov 27, 2017
phated added a commit that referenced this issue Nov 28, 2017
phated added a commit that referenced this issue Nov 28, 2017
phated added a commit that referenced this issue Nov 28, 2017
phated added a commit that referenced this issue Nov 28, 2017
phated added a commit that referenced this issue Nov 28, 2017
phated added a commit that referenced this issue Nov 28, 2017
phated added a commit that referenced this issue Nov 28, 2017
phated added a commit that referenced this issue Nov 28, 2017
phated added a commit that referenced this issue Nov 28, 2017
phated added a commit that referenced this issue Nov 28, 2017
phated added a commit that referenced this issue Nov 28, 2017
phated added a commit that referenced this issue Nov 28, 2017
phated added a commit that referenced this issue Nov 28, 2017
phated added a commit that referenced this issue Nov 28, 2017
phated added a commit that referenced this issue Nov 28, 2017
phated added a commit that referenced this issue Nov 28, 2017
phated added a commit that referenced this issue Nov 28, 2017
phated added a commit that referenced this issue Nov 28, 2017
phated added a commit that referenced this issue Nov 28, 2017
phated added a commit that referenced this issue Nov 30, 2017
phated added a commit that referenced this issue Nov 30, 2017
phated added a commit that referenced this issue Nov 30, 2017
phated added a commit that referenced this issue Nov 30, 2017
phated added a commit that referenced this issue Nov 30, 2017
phated added a commit that referenced this issue Dec 5, 2017
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

No branches or pull requests

4 participants