Skip to content
This repository was archived by the owner on Sep 11, 2018. It is now read-only.

Conversation

elliottsj
Copy link
Contributor

@elliottsj elliottsj commented Jun 8, 2018

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Because both webpack and webpack-command define a "bin" called "webpack" in their respective package.json manifests, there is ambiguity over which one will be linked at node_modules/.bin/webpack. When using pnpm, it turns out webpack's binary is linked rather than webpack-command's. So when node_modules/.bin/webpack is invoked, this is called:

require(installedClis[0].package);
// where installedClis[0].package === 'webpack-command'

This currently causes the command to output nothing because webpack-command's "main" module lib/index.js does not launch the CLI.

Changing the "main" field to reference lib/cli.js fixes this issue, the same as how it works for webpack-cli.

Breaking Changes

This breaks any consumers of webpack-command which expect require('webpack-command') to return the exports of lib/index.js. To avoid this breaking change, I can think of two approaches:

  1. Keep "main": "lib/index.js", but do some detection at the module scope of lib/index.js to detect whether it's being invoked as a CLI. I'm not too sure what this would look like though.
  2. Instead, change webpack/bin/webpack.js's implementation to import and execute some function exported by webpack-command, instead of relying on the side effects of require(installedClis[0].package);. This would introduce some inconsistency with how webpack-cli is invoked, so maybe some changes should be made over in webpack-cli too.

Use lib/cli.js as the package.json "main" field so that webpack-command
can be invoked via webpack/bin/webpack.js
@elliottsj
Copy link
Contributor Author

elliottsj commented Jun 8, 2018

To repro this issue using pnpm, you can run these commands:

npm install -g pnpm
mkdir pnpm-webpack-command-test
cd pnpm-webpack-command-test/
pnpm init -y
pnpm install -D webpack webpack-command
echo "console.log('test')" > index.js
./node_modules/.bin/webpack --mode none --entry ./index.js --output ./bundle.js
# (there's no output from invoking webpack)

@shellscape
Copy link
Contributor

Thanks for the PR/Issue 🍺

This is where I start to really dog on alternative package managers that start to redefine the rules. Shouldn't this be something that the pnpm project should resolve, as npm doesn't seem to have an issue?

As for webpack-command, the way the package is setup is the right way to go about that. cli.js is not the entrypoint, index.js is. cli.js is the file to which the binary is linked. This is a relatively standard setup. Given that webpack-command is supposed to be a CLI, and that's all its supposed to do, the main field should actually be removed. Though I'm not sure how this affects the pnpm issue.

@dhardtke
Copy link

dhardtke commented Jun 12, 2018

I just tested webpack-command and I think we're affected in our setup as well.

This is the way we invoke webpack:

node --max-old-space-size=8192 node_modules/webpack/bin/webpack.js --env.production --config config/webpack.prod.js --bail

with webpack-command it doesn't print anything :(

@shellscape
Copy link
Contributor

shellscape commented Jun 12, 2018

@dhardtke you're kind of off-topic here, and I dare say you're using that in an unintended manner. use the webpack-command bin directly with npx webpack, that's why npx exists. Also of note, the --env flag has been killed with fire in webpack-command - use environment variables instead of a hacky flag setup.

@elliottsj
Copy link
Contributor Author

I believe this isn't an issue with pnpm or npm necessarily, although it is a little annoying that neither emits a warning when there are multiple bins defined with the same name. The problem is that npm prioritizes webpack-command's bin while pnpm prioritizes webpack's bin, but ultimately I think this is an implementation detail. You can actually reproduce the problem with npm@6.1.0:

npm install webpack-command
npm install webpack
./node_modules/.bin/webpack --version
# no output

I agree that having "main": "lib/cli.js" doesn't quite feel like the right solution. What do you think about this direction instead?

Instead, change webpack/bin/webpack.js's implementation to import and execute some function exported by webpack-command, instead of relying on the side effects of require(installedClis[0].package);. This would introduce some inconsistency with how webpack-cli is invoked, so maybe some changes should be made over in webpack-cli too.

Or, another approach could be to rename webpack-command's bin to webpack-command, then change the webpack/bin/webpack.js implementation to simply exec ./node_modules/.bin/webpack-cli or ./node_modules/.bin/webpack-command (depending on what the user has installed)

@shellscape
Copy link
Contributor

This thread is raising some good points. I was never a fan of webpack's approach to the "things have moved to..." approach. I hope that's removed for webpack v5, and the cli modules can take it from there.

I think there's another approach that could be useful; a no-conflict approach. That's something that could be introduced in webpack has a postInstall script. What I'm imagining is something that looks for a recognized cli module (right now it's webpack-cli and webpack-command - but we've planned a deprecation and merge of webpack-cli) and then force link that binary as an added measure. That way it wouldn't matter which order the modules were installed, and that should cover any differences in precedence between package managers. Thoughts?

@elliottsj
Copy link
Contributor Author

That's an interesting idea. Although it doesn't quite feel like something that a well-behaved npm package would do, since you would expect that only npm itself would be changing the contents of node_modules/.bin/. I can imagine this might confuse some users, and we'd also need to make sure the script is compatible with pnpm, yarn, and alternatives, plus handle cases where the user is installing webpack and webpack-command globally.

@shellscape
Copy link
Contributor

Although it doesn't quite feel like something that a well-behaved npm package would do

Not typically, no. webpack made a poor architectural decision in how it went about moving the cli to a new, external project. I'm really of the opinion that's where the change (in whatever form) needs to happen. webpack-cli caving to modifying itself to get around that feels like putting a band-aid on an amputation and calling it a fix. Best to treat the root of the issue.

elliottsj pushed a commit to elliottsj/webpack that referenced this pull request Jun 14, 2018
Rather than require()-ing the "main" module in webpack-cli/
webpack-command, require() the "bin" module.

This avoids the issue described in
webpack-contrib/webpack-command#30
where installing packages in this order results in no output from
./node_modules/.bin/webpack:

    $ npm install webpack-command
    $ npm install webpack
    $ ./node_modules/.bin/webpack
    # exit 0 with no output
@elliottsj
Copy link
Contributor Author

elliottsj commented Jun 14, 2018

To summarize the options we've discussed so far:

Option Pros Cons
1. Use "main": "lib/cli.js". Simple; parity with webpack-cli. Breaking change since cli.js isn't the true entry point to webpack-command; index.js is.
2. Do some detection at the module scope of lib/index.js to detect whether it's being invoked as a CLI. Avoids breaking change; keep index.js as entry point. Seems hacky.
3. Change webpack/bin/webpack.js's implementation to import and execute some function exported by webpack-command, instead of relying on the side effects of require(installedClis[0].package);. No breaking changes. webpack/bin/webpack.js becomes coupled with webpack-command's API, rather than simply exec-ing a bin script.
4. Change webpack/bin/webpack.js's implementation to require() webpack-cli/webpack-command's bin script rather than the "main" module. No breaking changes. ???
5. Add a postinstall script to webpack, which will force-link the bin module of webpack-cli or webpack-command, depending on what the user has installed. No breaking changes. Non-standard, possibly fragile given the need to support npm alternatives, plus handle global installations.

I have an implementation of option 4 here: webpack/webpack#7534

@Legends
Copy link

Legends commented Jun 14, 2018

@shellscape

webpack made a poor architectural decision in how it went about moving the cli to a new, external project

reversible?

@shellscape
Copy link
Contributor

@Legends I'm sure it is.

@elliottsj given that you've opened a PR with webpack, we should probably close this one.

@elliottsj elliottsj closed this Jun 15, 2018
elliottsj pushed a commit to elliottsj/webpack that referenced this pull request Jun 15, 2018
Rather than require()-ing the "main" module in webpack-cli/
webpack-command, require() the "bin" module.

This avoids the issue described in
webpack-contrib/webpack-command#30
where installing packages in this order results in no output from
./node_modules/.bin/webpack:

    $ npm install webpack-command
    $ npm install webpack
    $ ./node_modules/.bin/webpack
    # exit 0 with no output
elliottsj pushed a commit to elliottsj/webpack that referenced this pull request Jun 15, 2018
Rather than require()-ing the "main" module in webpack-cli/
webpack-command, require() the "bin" module.

This avoids the issue described in
webpack-contrib/webpack-command#30
where installing packages in this order results in no output from
./node_modules/.bin/webpack:

    $ npm install webpack-command
    $ npm install webpack
    $ ./node_modules/.bin/webpack
    # exit 0 with no output
@elliottsj elliottsj deleted the webpack-bin-fix branch June 15, 2018 19:36
elliottsj pushed a commit to elliottsj/webpack that referenced this pull request Jun 25, 2018
Rather than require()-ing the "main" module in webpack-cli/
webpack-command, require() the "bin" module.

This avoids the issue described in
webpack-contrib/webpack-command#30
where installing packages in this order results in no output from
./node_modules/.bin/webpack:

    $ npm install webpack-command
    $ npm install webpack
    $ ./node_modules/.bin/webpack
    # exit 0 with no output
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants