Skip to content
This repository has been archived by the owner on Mar 31, 2020. It is now read-only.

Copy assets files when using mix.version() command #268

Closed
wants to merge 4 commits into from

Conversation

rafaelbeckel
Copy link

It's somewhat common to use relative paths in .css files.

Versioning such files with elixir invalidates all relative paths:
http://stackoverflow.com/questions/27789466/asset-management-maintaining-reference-to-relative-assets-after-concatenation/32223524#32223524

This PR proposes a simple syntax to solve this kind of problem, by allowing users to define an array of paths relative to /public as a second (or third) argument, so they'll be copied to /build after version command executes.

Usage

mix.version(
    ['css/style.css', 'css/vendor/style.css'], //files to be versioned
    'public/build', //build dir
    ['fonts', 'css/vendor/icons'] //dependent files/dirs to be copied
);

This will also re-copy every dependent files when using gulp watch.

It's somewhat common to use relative paths in .css files.

Versioning such files with elixir invalidates all relative paths:
http://stackoverflow.com/questions/27789466/asset-management-maintaining-reference-to-relative-assets-after-concatenation/32223524#32223524

This PR proposes a simple syntax to solve this kind of problem, by allowing users to define an array of paths relative to **/public** as a second (or third) argument, so they'll be copied to **/build** after `version` command executes.

**Usage**
```
mix.version(
    ['css/style.css', 'css/vendor/style.css'], //files to be versioned
    ['fonts', 'css/vendor/icons'] //dependent files/dirs to be copied
);
```

This will also re-copy every dependent files when using `gulp watch`.
Elixir.extend('version', function(src, buildPath) {
Elixir.extend('version', function(src, buildPath, assets) {
// Allow users to use assets as the second argument
if(Array.isArray(buildPath)) {
Copy link
Member

Choose a reason for hiding this comment

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

cs

@rafaelbeckel
Copy link
Author

Sorry for my ignorance, but what does a cs mean? Computer Science? Counter Strike? Commit Sucks? =D

@GrahamCampbell
Copy link
Member

code style

@rafaelbeckel
Copy link
Author

OK, thanks for spotting.

So what is the recommended style then?

Does this project have a styleguide published somewhere?

@rafaelbeckel
Copy link
Author

I mean, which is the specific issue in this case?

It's something like "there are cleaner ways to detect arrays in js" or "arguments should have only one well-defined responsability, do not ever try to hook this kind of kludge!"

As I think the second one is most probable, I can just remove this block (the rest will work just fine), but users would need to call all arguments on gulpfile, even if they don't want to change the build dir.

I personally wouldn't mind to write mix.version('files',null,['copied','stuff']); or mix.version('files','build',['copied','stuff']) instead of mix.version('files',['copied','stuff']).

rafaelbeckel and others added 2 commits September 18, 2015 04:00
There was a refactor from @JeffreyWay in this file on main repository before my PR was accepted.

So, I solved the resulting conflicts and developed a cleaner version of the proposed feature, based on his last commit.

Also, I removed the option for ommiting the second argument. While slightly handy for the end user, the implementation was not nice code.
@rafaelbeckel
Copy link
Author

There was a refactor from @JeffreyWay in this file on main repository before this PR was accepted.

So, I solved the resulting conflicts and developed a cleaner version of the proposed feature, based on his last commit.

Also, I removed the option for ommiting the second argument. While slightly handy for the end user, the implementation was not nice code.

@JeffreyWay
Copy link
Contributor

I'm not sure about this.

Isn't it a best practice to not use relative paths in your CSS files?

@rafaelbeckel
Copy link
Author

Hey Jeff,

Yes, it is a bad practice and our team does not use relative paths in our controlled files.

However, there are some vendors that do it, and we do not touch their codes - everything is just controlled via Bower.

Things started to break since we started versioning our files. So, this PR was the solution we've found to solve the problem, while keeping a clean and readable gulpfile. We are using it in production since then.

We use this third (optional) argument to copy some small files (i.e fonts and icons) used by some third-party libs that use relative paths inside their own directory structure.

We could use mix.copy() command to do that. However, it does not register the same watcher, so it invalidates gulp watch, forcing developers to recompile the entire stack again and again while developing. Another possible solution would be changing vendor files to use a fixed path, but it would be harder to maintain.

@JeffreyWay
Copy link
Contributor

I really appreciate the PR - and may merge at a later date. But it doesn't seem like anyone else needs this.

And I'd prefer to keep the versioning API as simple as possible, rather than passing a special array as an arg.

Thanks!

@JeffreyWay JeffreyWay closed this Nov 2, 2015
@rafaelbeckel
Copy link
Author

Well... maybe a callback would be a better idea, instead of an array?

That way people could implement whatever hook they need.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
3 participants