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

Ported support for nosuffix from master to 1.6.x. #39

Merged
merged 1 commit into from
Feb 22, 2014

Conversation

timothystone
Copy link

Ported the nosuffix support from 1.7 to 1.6. Hope it's useful (I see a debugging statement or two I left in while I was figuring out how the code worked). Works as intended in a local Stripes project.

@timothystone
Copy link
Author

Sam,

Any chance this support for no suffix could be applied to the 1.6.x branch?

Thanks,
Tim

@samaxes
Copy link
Owner

samaxes commented Feb 22, 2014

Hi @timothystone,

Sorry taking so long to reply back to you.
I've pulled this commit locally and run it against the demo application and it's not working properly for me.
Can you please try this on the demo folder and check if you have any content on both the CSS and JS folders bellow the target directory?

Cheers

@samaxes samaxes merged commit bb10d41 into samaxes:v1.6.X Feb 22, 2014
@samaxes
Copy link
Owner

samaxes commented Feb 22, 2014

@timothystone, I had to do some changes to have it working on the demo folder.
All the changes have been pushed to the v1.6.X branch.
Can you please try that code before I deploy version 1.6.2 to Maven central?

@timothystone
Copy link
Author

So I can report that this patch is working in the build of two consumer facing applications for an international credit issuer. :)

Did you find something after your comment that prompted the merge anyway? If you corrected it, awesome.

@timothystone
Copy link
Author

I will find an excuse this weekend to VPN and pull 1.6.x locally for a build and test.

@samaxes
Copy link
Owner

samaxes commented Feb 22, 2014

Maybe you had additional changes on your side.

On the pull request, the .tmp files were being cleaned in two different places.

  1. On the ProcessFilesTask class:
if (nosuffix) {
    if (!mergedFile.delete()) {
        mergedFile.deleteOnExit();
    }
}
  1. And by calling cleanupFiles on ProcessCSSFilesTask and ProcessJSFilesTask classes.

Removing the method cleanupFiles was enough to make it work (hopefully not only on the demo application).

Thanks for the help!
And sorry again for taking so long to merge your code.

@samaxes samaxes self-assigned this Feb 22, 2014
@samaxes samaxes added this to the 1.6.2 milestone Feb 22, 2014
@timothystone
Copy link
Author

No question that finding is my tackling the code with a flash light and not the flood light of the original dev. :) I was testing against our Maven projects (using Stripes BTW). Our project structure might have been forgiving of my calls, but it might also explain why I had to isolate the plugin execution to a profile so as not to hose the users working copy. The minify goal is only called on our build box and not in local development.

I will definitely take a look this weekend. Monday at the latest.

@samaxes
Copy link
Owner

samaxes commented Feb 22, 2014

Probably the same issue I had :)
When setting both nosuffix and skipMerge options to true, the original CSS and JS files were being overridden with the plugin output.

@samaxes
Copy link
Owner

samaxes commented Feb 27, 2014

Hi @timothystone, have you had the time to try and test the latest changes on the v1.6.X branch?

@timothystone
Copy link
Author

I will log on and test tonight.

@timothystone
Copy link
Author

HOLD. I need to test it again. Something odd happened and it looked good, but the WAR does not have the compressed files and the target reverted to uncompressed.

@timothystone
Copy link
Author

So something is happening and the target directory is refreshing with the uncompressed CSS and JS files. Looking at the debug build.

@timothystone
Copy link
Author

I'll have to review in the AM.

@samaxes
Copy link
Owner

samaxes commented Mar 4, 2014

Probably the same issue as in #50.
Change the phase to package and it should run just fine.
I've just published the release v1.6.2 to maven central repo.
Thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants