jonkemp / useref

Parse build blocks in HTML files to replace references
MIT License
51 stars 13 forks source link

Feature: Alternate source path #4

Closed haensl closed 8 years ago

haensl commented 8 years ago

I implemented the option to specify an alternate source path for js and css blocks. This feature allows one to specify a different path to be set in src or href attributes of the generated output and comes in handy when working with routes that deviate from the actual file path, e.g. in an Express application. The alternate source path can be specified after the output file path, e.g.:

<!-- build:js /path/to/file.js(/alternate/path.js) -->
<script src="/a/file.js"></script>
<!-- endbuild -->

becomes:

<script src="/alternate/path.js"></script>

whereby /a/file.js is still written to /path/to/file.js.

jonkemp commented 8 years ago

Closing. Please read the Contributing doc.

https://github.com/jonkemp/useref/blob/master/CONTRIBUTING.md#pull-requests

In addition, to not asking or discussing it before coding this feature, you've changed the formatting of the README, changed formatting in the code and did not respect the code style, bumped the version (incorrectly) and added yourself as a contributor. I will not accept this pull request as it was made so I will go ahead and close it.

If you would like to try again, please read the Contributing doc and make a feature request first. If a pull request is welcome, do not change formatting of code unless it fits with the code style or other files like a README and do not add yourself as a contributor or otherwise modify settings in the package.json unless it is adding a dependency or something.

haensl commented 8 years ago

Hi there, sorry for the inconvenience! I did not make a feature request, since I didn't want to bother you with implementing it. Sorry for the formatting screw up in the README, I guess that's caused by my markdown linter. As to incorrectly bumping the version: can you explain what's incorrect with bumping the feature part of a semantic versioning scheme for a new feature? If I find the time, I'll redo README and package.json, as I was not aware that adding oneself as a contributor when contributing and updating the version is something not welcome to be done in a pull request.

Cheers,

HP

jonkemp commented 8 years ago

Bumping the package version if the version is major or minor should set the patch version to 0. IMO, changes to the project metadata in package.json is usually outside the scope of a pull request and should be done by the maintainers.

Yes, in the future anything that would warrant a version bump, major or minor, should be discussed first. Thanks.

haensl commented 8 years ago

Oh, I see. Not resetting the minor is my mistake :) - sorry for that. I'm currently resetting the README and the package.json, but I have one more question: Why the inconsistency with object properties in the JS? Sometimes you use Strings as property names, e.g. in many/most module.exports, other times e.g. in parseBuildBlock.js exports you don't. As far as I know it is not recommended to use strings as property names in JS.

Anyhow, before I create a feature request and another pull request, do you even want that feature in the project? Otherwise I'll just use my fork.

jonkemp commented 8 years ago

Why the inconsistency with object properties in the JS?

Just an oversight I was unaware of. I just completed a large refactor and probably just copied that somehow and continued it. Ideally, that should be something a linter would catch. I will see if there is an ESLint rule. Thanks for pointing it out.

As far as this feature goes, I can't imagine why it would be useful. So I don't understand the use case. I am hesitant about adding functionality that will add complexity when it is already very complex unless a large number of users would find it useful. This is why a feature request is important so others can weigh in. In addition, a good pull request or feature request should convince the maintainer why it's needed.

haensl commented 8 years ago

OK, thanks, then I won't go through the trouble of requesting the feature as it is easily implemented, doesn't add much complexity and I want/need it. I just thought you might be interested in including this functionality.

As for the use case: Imagine you have a route on your node server for js which differs from the actual file path. E.g. I address something via

<script src="/a/folder/file.js"></script>

and my route maps /a/folder to /another/folder. Then I want the generated file to be put into /another/folder but the src attribute of the <script> tag to point to /a/folder. This can easily be accomplished by adjusting the regbuild regex and the target in your implementation, as can be seen in my changes.