Wrong parsing of arguments in install-node-and-nmp

[janzyka]

I need to provide custom .npmrc to the npm install command. The .npmrc is located in root of my project. My plugin configuration looks as:

<!--Install dependencies using npm-->
<execution>
    <id>npm install</id>
    <goals>
        <goal>npm</goal>
     </goals>
     <configuration>
         <arguments>install --userconfig "${basedir}/.npmrc"</arguments>
     </configuration>
 </execution>

The quotes "" are provided because this runs on my Jenkins CI server and it constructs the temp folder for CI build out of project name which contains spaces. Unfortunately the the args are simply split on \s+ which ignores the quotes hence cannot be used with custom .npmrc located in path which contains spaces. The issue can be seen here:

https://github.com/eirslett/frontend-maven-plugin/blob/master/frontend-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/NodeTaskExecutor.java#L79

IMHO the problem is you take all the arguments as single String. I suggest you have option to provide List of arguments instead and take them as they are. To make the change backward compatible you can provide new list property like <argumentsList> and use the one which is set, fail if both are set. The fixed <configuration /> may look like:

<!--Install dependencies using npm-->
<execution>
    <id>npm install</id>
    <goals>
        <goal>npm</goal>
     </goals>
     <configuration>
         <argumentsList>
            <argument>install</argument>
            <argument>--userconfig</argument>
            <argument>"${basedir}/.npmrc"</argument>
         <argumentsList>
     </configuration>
 </execution>

Overall doing split on space looks dangerous to me. Let me know if you need help with this, will be glad to assist.

[eirslett]

Would it be an option to split on space except when there are quotes? Is there a simple Java way to do that?

[janzyka]

This is tricky, what you are looking for is really some command line parser. The quotes may be escaped etc. There is whole apache project for that.

As I see it you want to pass arguments to a process. This is really Collection of Strings. Even you hit the problem that the underlying API wants you to pass in a Collection of parameters rather a single String - therefore you did the split. If user has a way to provide directly the collection that would be easiest from my point of view.

Again, if you are OK with that I can do the coding a pull request.

[eirslett]

Ok, let's go for it.
What I would like to see, is that the old syntax is "supported" as input for the maven plugin, but if set, it should throw an exception that fails the build fast, and provides an explanation (something along the lines of use argumentsList instead of arguments). So the plugin doesn't have to support both of them, which can become a burden in the long run.
The examples and integration tests would also have to be updated.

[otakun]

Any news on this issue?
I suggest to offer entering a list of arguments mentioned above. That's at least the bulletproof way to do it.

[ghuser]

+1 for fixing this issue

[buuhuu]

See the linked PR. It resolves the issue without introducing a new configuration but makes parsing the arguments list a bit smarter. Introducing a new configuration is quite an effort compared to what is proposed here as a couple of places have to be touched. With that PR only the logic of 2 well defined methods has been changed and testing guarantees that the (proper) behaviour as it has been in place before has not been changed. The breaking change of the misbehaviour is probably a minimal risk from my perspective.