Add prettier-eslint as a peer dependency and add an npm prepare script to forward to nps build

[jondkinney]

In order to allow prettier-eslint-cli to point to the master branch of prettier-eslint to account for this issue: https://github.com/prettier/prettier-eslint/pull/194, I needed to remove the explicit dependency, add it back in the peerDependencies section. This allowed me to specify the version of prettier-eslint that I wanted to use in my project's main package.json file. The version I wanted to use was the master branch of github since it has a fix that isn't yet officially deployed: https://github.com/prettier/prettier-eslint/pull/194

I also had to add the prepare npm script to forward through to nps so that the package would have it's build scripts run when being installed from github per this features: https://blog.jim-nielsen.com/2018/installing-and-building-an-npm-package-from-github/#installing-and-building-packages-with-npm-from-github

I have another PR for prettier-eslint here that is complementary. Both PRs will need to be merged to use these packages together to fix the warning mentioned in https://github.com/prettier/prettier-eslint/pull/194.

Without specifying prettier-eslint as a peerDependency of prettier-eslint-cli the problem is that prettier-eslint-cli will install its own version of prettier-eslint and that old version doesn't have the fix specified in https://github.com/prettier/prettier-eslint/pull/194

If I'm making this way harder than it needs to be please let me know. But this all seems to work. I only have one version of prettier-eslint installed in my node_modules folder. prettier-eslint-cli's node_modules folder does not have its own prettier-eslint, and the error is gone!

[bj00rn]

👍

would be awesome to have prettier-eslint as peer-dep over having to wait for releases

@zimme what you think?

[bj00rn]

for CI errors see #150 ..

[zimme]

In this case, even if prettier-eslint-cli is installing it's own version of prettier-eslint as your dependency of prettier-eslint is on the project layer I believe that prettier-eslint-clis import statements will actually import your version of prettier-eslint but I could be wrong here... I'm not too sure of the import internals.

Another approach to adding prettier-eslint as a peerDependency is to use require.resolve for prettier-eslint as we do for eslint and prettier in the prettier-eslint code.

This will allow prettier-eslint to be installed as a dependency of prettier-eslint-cli but if you have a different version installed as a project dependency that will be the one resolved and imported and it will only fall back to it's own verison if it couln't find any prettier-eslint on the project level dependencies.

[bj00rn]

@zimme so what's the verdict, can we have his merged?

Seems resonable to me to keep prettier-eslint in peer-deps, rather than having to wait for new releases of prettier-eslint-cli

[zimme]

My initial thoughts is that, could we look into using require.resolve as it's done in prettier-eslint, before merging this.

It's nice to just have to do npm install -D prettier-eslint-cli and automatically get prettier-eslint.

But I do believe more people are getting used to installing peer deps these days, so this is the way to go if the require.resolve approach isn't applicable in this situation.

[codecov-io]

Codecov Report

Merging #144 into master will not change coverage. The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #144   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines         123    123           
  Branches       16     16           
=====================================
  Hits          123    123

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c8c077b...14dc622. Read the comment docs.

[bj00rn]

@zimme im not sure i follow... something like so: https://github.com/bj00rn/prettier-eslint-cli/commit/1885d283e2f69c8a9059b03d2c6bd50bfff02e32

[zimme]

Yeah, that seems to look like what we do in prettier-eslint

[zimme]

I've updated prettier-eslint and eslint in version 5.0.0.

Let's @bj00rn let's test this approach next.

[sarendipitee]

@zimme I'd really like to see this project move to a peerDep route b/c it is not being maintained well (including eslint itself). Your bump to [email protected] was nearly a year ago.

For example, today I ran across this issue:

Error: ESLint configuration in .eslintrc.js is invalid:
	- Unexpected top-level property "ignorePatterns".

Which is pretty straight forward — the version of eslint that prettier-eslint-cli uses is outdated and doesn't have the new ignorePatterns feature shipped.

[friederbluemle]

Any updates here? I think having prettier-eslint as a peer dependency has more advantages than disadvantages.

[kylemh]

@bj00rn any context here could help. Did you follow through on https://github.com/prettier/prettier-eslint-cli/pull/144#issuecomment-502588064

Should this be merged?

[friederbluemle]

A couple of questions:

1. Can the PR head branch be rebased? Right now, messageformat, rxjs, and yargs appear downgraded in the diff.
2. Moving prettier-eslint to peerDependencies makes sense, but should it be restricted to ^9.0.0? Or would a wider range be possible? I didn't check, so unclear if there are major breaking changes in prettier-eslint between major versions (the current version is 12.0.0).

[kylemh]

1. Was an honest mistake! Easy to correct.

2. I'll have to dig a bit deeper this weekened.

[JounQin]

I'm against a bit to mark a dependency as peer when it's a cli tool, especially for global installation.

The cli must use correct version of prettier-eslint, so it's not peer.

[kylemh]

peer versions are still required. it seems in the past this CLI has supported a range of versions of prettier-eslint at a time. This means prettier-eslint is ideally a peerDep in my book.

[JounQin]

peer versions are still required. it seems in the past this CLI has supported a range of versions of prettier-eslint at a time. This means prettier-eslint is ideally a peerDep in my book.

It make a single npx prettier-eslint-cli unavailable then.

Peer dependency is not good for cli tools IMO.

[kylemh]

I disagree 🤷🏼

prettier-eslint-cli is a tool that runs prettier-eslint from the CLI. It's an abstraction atop another tool. Embedding the other tool seems like a mistake. It's like glueing socks into a shoe.

Ignoring metaphors, realize that people may have separate use cases for prettier-eslint AND prettier-eslint-cli in one repository. By making it a dep instead of a peerDep, we become an obstacle. I think it's much better to require a valid range of the necessary dep.

[JounQin]

I think it's much better to require a valid range of the necessary dep.

Would >=x.y.z but as dependency help?

[kylemh]

No, because the package would be bundled with one version regardless of the expressed range in package.json

[JounQin]

No, because the package would be bundled with one version regardless of the expressed range in package.json

Something like yarn-deduplicate --strategy fewer may be very suitable for this. See also https://github.com/scinos/yarn-deduplicate#deduplication-strategies

It's a node package, and the dependencies are not bundled, they can still be deduplicated.

A dirty solution could be we install prettier-eslint as original-prettier-eslint@npm:prettier-eslint and mark prettier-eslint as an optional dependency, then we try/catch to get user defined prettier-eslint first and fallback to use internal original-prettier-eslint, and all users would be happy, right?

I don't know whether it's a good solution for cli tools.

[kylemh]

Or we could just list it as a peerDep!

dependencies are not bundled

But they are in node_modules which can affect module resolutions of other deps.

[JounQin]

Or we could just list it as a peerDep!

As I said previously, adding it a peer only is not kindly for a cli tool. npx prettier-eslint-cli is unavailable to go in this case.

But they are in node_modules which can affect module resolutions of other deps.

Right, that's why I point yarn-deduplicate, and provide the dirty solution.

[kylemh]

I've been using google to figure out the installation behavior of npx, but I'm not finding anything to suggest that npx ignores peerDependencies when it installs the binary's dependencies. Do you have a source to back this up:

As I said previously, adding it a peer only is not kindly for a cli tool. npx prettier-eslint-cli is unavailable to go in this case.

Frankly, my opinion is that I'd rather not support npx (and force users to install the dependency) than force users to use a certain version of a dependency. It's a lot easier to install the prettier-eslint-cli before using it than it is to use a different version of a package it lists in dependencies.

[JounQin]

I've been using google to figure out the installation behavior of npx, but I'm not finding anything to suggest that npx ignores peerDependencies when it installs the binary's dependencies. Do you have a source to back this up:

I didn't find either, but the official document is here: https://docs.npmjs.com/cli/v8/commands/npx#description

And I found yarn dlx does not install peer automatically: https://github.com/yarnpkg/berry/issues/2268#issuecomment-752804430

And also, npm@v7+ installs peer dependencies automatically, but it could be harmful: https://stackoverflow.com/questions/66239691/what-does-npm-install-legacy-peer-deps-do-exactly-when-is-it-recommended-wh

Frankly, my opinion is that I'd rather not support npx (and force users to install the dependency) than force users to use a certain version of a dependency. It's a lot easier to install the prettier-eslint-cli before using it than it is to use a different version of a package it lists in dependencies.

Personally, I don't want to enforce the users to do something unexpectedly.

Is there any downside if we go with my dirty solution here? We'll support peer with/without at the same time.

---

Maybe a PR is good for reviewing.

[kylemh]

Is there any downside if we go with my dirty solution here? We'll support peer with/without at the same time.

Yes, people that use both prettier-eslint and prettier-eslint-cli would not readily be told that they're using potentially different versions of the formatter.

[JounQin]

Yes, people that use both prettier-eslint and prettier-eslint-cli would not readily be told that they're using potentially different versions of the formatter.

I'm not sure to understand. If prettier-eslint is installed by the user, the user installed version will be used (We're using original-prettier-eslint@npm:prettier-eslint inside, so it will not install different prettier-eslint versions).

Never mind, wait for my PR please.

[kylemh]

The binary would reference the version installed by prettier-eslint-cli. If somebody also is using prettier-eslint separately from the CLI, they'd use whatever version they installed. Different versions.

[JounQin]

The binary would reference the version installed by prettier-eslint-cli. If somebody also is using prettier-eslint separately from the CLI, they'd use whatever version they installed. Different versions.

No, it will not.

Wait for my PR please.

[kylemh]

No, it will not.

It definitely will if they use both prettier-eslint and prettier-eslint-cli as dependencies. If prettier-eslint-cli is used via npx sure, you're correct there.