Open 123Haynes opened 1 year ago
This PR should also fix https://github.com/eirslett/frontend-maven-plugin/issues/1105 since yarn accepts the same syntax.
@eirslett can you take a look when you have time? 😃
I agree that the extra npm arguments are being added on the wrong side of the command that npx is running. The Javadoc for the NpxRunner states npm arguments, that need to be split from the npx arguments by '--'.
However, the docs for the npx command show that this separator is meant to come before the command and its arguments that npx is running:
npx --package=<pkg>[@<version>] -- <cmd> [args...]
As this issue states, the reverse is happening. --registry
and related options are being added at the end. What ends up happening is that the command sees these extra options and doesn't know what to do with them and may fail.
Here's a reproducible case to demonstrate
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<groupId>org.example</groupId>
<artifactId>frontend-maven-plugin-example</artifactId>
<version>1.0.0-SNAPSHOT</version>
<packaging>pom</packaging>
<build>
<plugins>
<plugin>
<groupId>com.github.eirslett</groupId>
<artifactId>frontend-maven-plugin</artifactId>
<version>1.15.0</version>
<configuration>
<environmentVariables>
<npm_config_cache>${project.build.directory}/_npx</npm_config_cache>
</environmentVariables>
<installDirectory>${project.build.directory}</installDirectory>
<nodeVersion>v20.12.2</nodeVersion>
</configuration>
<executions>
<execution>
<id>install-node</id>
<goals>
<goal>install-node-and-npm</goal>
</goals>
<phase>initialize</phase>
</execution>
<execution>
<id>run-npx</id>
<goals>
<goal>npx</goal>
</goals>
<phase>process-resources</phase>
<configuration>
<arguments>--yes --package js-yaml js-yaml test.yml</arguments>
</configuration>
</execution>
</executions>
</plugin>
</plugins>
</build>
</project>
First, create the test.yml file to load.
test.yml
key: value
If you run the following command, it will work:
mvn clean process-resources
However, if you pass an npm registry URL:
mvn clean process-resources -DnpmRegistryURL=https://registry.yarnpkg.com
here's what it will run:
npx --yes --package js-yaml js-yaml test.yml -- --registry=https://registry.yarnpkg.com
Notice that the --registry
option comes after the command that npx is running. As a result, the js-yaml command will report the following error message:
js-yaml: error: unrecognized arguments: --registry=https://registry.yarnpkg.com
There are many commands that don't recognize these extra arguments that are supposed to be passed to npm itself. Here's how the command should look to solve this problem:
npx --registry=https://registry.yarnpkg.com --yes --package js-yaml js-yaml test.yml
There's also no need for the --
separator since npx handles that automatically based on when it sees the first positional argument.
Summary
This PR fixes #1030 fixes #1121 fixes #1105 in the current version of the plugin the npx goal is broken when using a proxy.
As you can read in the npm documentation npx does not support the syntax that was used by the plugin before: https://docs.npmjs.com/cli/v8/commands/npm-exec#npx-vs-npm-exec
syntax before:
npx @sompackage arg1 arg2 -- --proxy=someproxy --https-proxy=someproxy --registry=someregistry
syntax after:
npx --proxy=someproxy --https-proxy=someproxy --registry=someregistry @somepackage arg1 arg2
npm
supports both syntax so no harm done in switching it.Tests and Documentation
I've changed the 3 existing tests to check for the new syntax. Please let me know if you need other changes. I'll try to get to it asap 😃