Move parsing-related dependencies to peer dependencies. #505

kylemh · 2021-02-28T19:23:30Z

codecov-io · 2021-02-28T19:27:09Z

Codecov Report

Merging #505 (badfbd3) into master (6d20e97) will decrease coverage by 0.92%.
The diff coverage is 84.61%.

@@             Coverage Diff             @@
##            master     #505      +/-   ##
===========================================
- Coverage   100.00%   99.07%   -0.93%     
===========================================
  Files            2        2              
  Lines          262      217      -45     
  Branches        67       43      -24     
===========================================
- Hits           262      215      -47     
- Misses           0        2       +2

Impacted Files	Coverage Δ
src/index.js	`97.61% <84.61%> (-2.39%)`	⬇️
src/utils.js	`100.00% <0.00%> (ø)`

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 6d20e97...5be12c6. Read the comment docs.

zimme · 2021-02-28T20:12:31Z

All the "extra" parsers should also be marked as optional peer dependencies using peerDependenciesMeta. This way npm 7 won't automatically install those peer dependencies that this package don't need when just formatting js files.

hamzahamidi · 2021-02-28T20:12:38Z

README.md

-[fix]: http://eslint.org/docs/user-guide/command-line-interface#fix
-[npm]: https://www.npmjs.com/
-[node]: https://nodejs.org
+[all-contributors]: https://github.com/kentcdodds/all-contributors


Not sure why you changed the order of the links

Alphabetized. No real reason for it. Just felt awkward slapping the first-defined links at the end of the list.

zimme · 2021-02-28T20:27:42Z

Just a quick note on dependencies in npm and my view of it.

For this package I don't really think peer dependencies is the way to go because of my understanding of peer deps.

If someone don't have prettier or eslint in their project, I want them to just be able to install this project and get the latest supported versions of eslint and prettier automatically.

With npm 7+ this will actually be the case when using peer deps as they will be installed automatically unless they're marked as optional.

However, my reservation of using peer deps is also based on the fact that this package doesn't produce any exports that are intended to be used by prettier or eslint unlike eslint-config-prettier.
This package actually depends on prettier and eslint and needs them to be installed and if they're not in the project should install them. We should however maybe be less restrictive with the version that we depend on if we can't be bothered to update it in a timely manner.

So from my understanding peer deps isn't about "bring your own <package>" but more about "I produce something that you will use in <package> so you need to use it in your project". It's a small difference but still a difference.

Then a note on the optional dependencies. I don't really see what the problem with using optional dependencies based on your comment in #402.

edit:

PS. I'll leave this decision up to you as I don't really use this project any longer and is working on a more general way of using multiple linters/formatters together at https://github.com/linterjs, which I don't have time to work on either 🤦

kylemh · 2021-02-28T20:36:33Z

I know this project isn't a focus for you, so I appreciate the attention you're giving it for us to get this PR out.

I'm no expert on this, so I could totally be wrong, but I really feel like peerDeps is the correct place.

If someone don't have prettier or eslint in their project, I want them to just be able to install this project and get the latest supported versions of eslint and prettier automatically.

I feel like this is problematic, because we'll be left - as maintainers - to constantly update the shipped parsers.

With npm 7+ this will actually be the case when using peer deps as they will be installed automatically unless they're marked as optional.

With every other package manager, this is NOT the case. It was actually a large point of contention: https://twitter.com/housecor/status/1325524945334661121
https://twitter.com/arcanis/status/1325727984238665728

Then a note on the optional dependencies. I don't really see what the problem with using optional dependencies based on your comment in #402.

Aside from my comment in #402, semantically "optionalDependencies" is incorrect.

If a dependency can be used, but you would like npm to proceed if it cannot be found or fails to install, then you may put it in the optionalDependencies object.

Our project cannot work without those dependencies.

zimme · 2021-02-28T20:48:13Z

This project can work without the ts and vue parser and those were the ones I was suggesting to go in optionalDependencies.

kylemh · 2021-02-28T20:55:12Z

Oh! I didn't realize. Okay, I agree those are optional, but I feel like we still have it right as-is.

Let's say somebody wants to use their own version of vue-eslint-parser. If we moved it to optional, wouldn't they be required to do npm i --no-optional which makes decisions about other dependencies for them?

zimme · 2021-02-28T20:57:54Z

Nope, it's an optional dependency for this package so this package won't crash and burn if it's not present, if they add it as a (dev)dependency in their project which is using this package, then we'll be able to pick it up and use it too.

However I do believe you might need to look over the require logic for those dependencies if you're changing them to be optional (peer) dependencies as the current logic assumes it will be able to import them?

kylemh · 2021-02-28T21:00:44Z

  if (['.ts', '.tsx'].includes(fileExtension)) {
    formattingOptions.eslint.parser =
      formattingOptions.eslint.parser ||
      require.resolve('@typescript-eslint/parser');
  }

  if (['.vue'].includes(fileExtension)) {
    formattingOptions.eslint.parser =
      formattingOptions.eslint.parser || require.resolve('vue-eslint-parser');
  }

I've got nothing on this 😬

You're suggesting this might not work if I move them to optional deps?

zimme · 2021-02-28T21:04:20Z

It will try and use any parser that the user provides via options (which might error, but then it's their responsibility) and fallback to using the one we've installed for them, as they're currently not optional, the require.resolve call will throw an error if it's not installed.

If the module can not be found, a MODULE_NOT_FOUND error is thrown.

From the require.resolve docs.

So this would need to be surrounded by a try catch that just ignores the MODULE_NOT_FOUND error.

zimme · 2021-03-01T05:54:14Z

Pretty sure the commit that wraps require.resolve with try catch also removes the ability for users to specify their own parser.

kylemh · 2021-03-01T06:21:42Z

Shit. Duh. Sorry. Fix in the morning.

kylemh · 2021-03-06T19:55:06Z

Some updated dependency is logging a warning in test:

No parser and no filepath given, using 'babel' the parser now but this will throw an error in the future. Please specify a parser or a filepath so one can be inferred.

zimme · 2021-03-06T20:00:53Z

I believe we have a test that does not provide either a filepath or a parser, so prettier logs this warning as it can't use a specific parser or infer the parser based on file extension.

Not sure if this is a prettier 1 or 2 warning though and maybe we should remove support for processing without either parser or filepath.

zimme · 2021-03-06T20:02:43Z

I think this was supported to allow a new file in an editor, which haven't been saved yet and no syntax selected to be formatted with prettier as if it were a js file

kylemh · 2021-03-06T20:03:19Z

There's also still something wrong with this PR 🤔

On master npm run test doesn't fail, but seems to have the same output. Will dig in more.

I like the idea of that breaking change in conjunction with this PR though.

…e-deps-to-peer-deps

kylemh · 2021-03-07T00:16:44Z

It's because of missing coverage, and I'm not quite sure how to get those last bits of coverage. Comments in test code. I'll take another stab at this and the breaking change some time later!

codecov-commenter · 2021-07-26T16:21:59Z

Codecov Report

Merging #505 (badfbd3) into master (f74350a) will decrease coverage by 0.92%.
The diff coverage is 84.61%.

❗ Current head badfbd3 differs from pull request most recent head b4173d3. Consider uploading reports for the commit b4173d3 to get more accurate results

@@             Coverage Diff             @@
##            master     #505      +/-   ##
===========================================
- Coverage   100.00%   99.07%   -0.93%     
===========================================
  Files            2        2              
  Lines          262      217      -45     
  Branches        67       43      -24     
===========================================
- Hits           262      215      -47     
- Misses           0        2       +2

Impacted Files	Coverage Δ
src/index.js	`97.61% <84.61%> (-2.39%)`	⬇️
src/utils.js	`100.00% <0.00%> (ø)`

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 f74350a...b4173d3. Read the comment docs.

github-actions · 2022-12-16T00:03:51Z

Stale pull request

kylemh added 3 commits February 28, 2021 11:00

docs: update README

ddb0a7b

move parsing-related deps to "peerDependencies"

6b7f86a

update installation instructions

e135b57

kylemh mentioned this pull request Feb 28, 2021

Support TypeScript v4.x #402

Closed

kylemh requested review from hamzahamidi and zimme February 28, 2021 19:45

mark extra parsers as optional

9b7e1d0

hamzahamidi approved these changes Feb 28, 2021

View reviewed changes

wrap require.resolve calls in try/catch

62a5fab

kylemh force-pushed the move-deps-to-peer-deps branch from 6df49c1 to 62a5fab Compare February 28, 2021 23:06

kylemh and others added 3 commits March 1, 2021 09:02

always choose custom parser if passed

924c3e2

Update index.js

087e150

fix typo

ec12e18

kylemh added 2 commits March 6, 2021 13:54

Merge branch 'master' of github.com:prettier/prettier-eslint into mov…

f5facd5

…e-deps-to-peer-deps

allow personalized vscode settings

8e0f3ba

kylemh added 4 commits March 6, 2021 14:49

format code

fed71f3

chore: add test for passed parser being consumed

693df46

add @types/jest selfishly

59848c5

add some work regarding tests

5be12c6

Merge branch 'master' into move-deps-to-peer-deps

b4173d3

kylemh mentioned this pull request Apr 18, 2022

feat: ✨ Support ESLint 8 #696

Merged

github-actions bot added the no-pr-activity label Dec 16, 2022

kylemh removed the no-pr-activity label Dec 17, 2022

kylemh closed this Dec 17, 2022

JounQin deleted the move-deps-to-peer-deps branch December 25, 2023 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move parsing-related dependencies to peer dependencies. #505

Move parsing-related dependencies to peer dependencies. #505

kylemh commented Feb 28, 2021

codecov-io commented Feb 28, 2021 •

edited

Loading

zimme commented Feb 28, 2021 •

edited

Loading

hamzahamidi Feb 28, 2021

kylemh Feb 28, 2021

zimme commented Feb 28, 2021 •

edited

Loading

kylemh commented Feb 28, 2021 •

edited

Loading

zimme commented Feb 28, 2021

kylemh commented Feb 28, 2021

zimme commented Feb 28, 2021

kylemh commented Feb 28, 2021

zimme commented Feb 28, 2021 •

edited

Loading

zimme commented Mar 1, 2021

kylemh commented Mar 1, 2021

kylemh commented Mar 6, 2021 •

edited

Loading

zimme commented Mar 6, 2021

zimme commented Mar 6, 2021

kylemh commented Mar 6, 2021 •

edited

Loading

kylemh commented Mar 7, 2021

codecov-commenter commented Jul 26, 2021

github-actions bot commented Dec 16, 2022

Move parsing-related dependencies to peer dependencies. #505

Move parsing-related dependencies to peer dependencies. #505

Conversation

kylemh commented Feb 28, 2021

codecov-io commented Feb 28, 2021 • edited Loading

Codecov Report

zimme commented Feb 28, 2021 • edited Loading

hamzahamidi Feb 28, 2021

Choose a reason for hiding this comment

kylemh Feb 28, 2021

Choose a reason for hiding this comment

zimme commented Feb 28, 2021 • edited Loading

kylemh commented Feb 28, 2021 • edited Loading

zimme commented Feb 28, 2021

kylemh commented Feb 28, 2021

zimme commented Feb 28, 2021

kylemh commented Feb 28, 2021

zimme commented Feb 28, 2021 • edited Loading

zimme commented Mar 1, 2021

kylemh commented Mar 1, 2021

kylemh commented Mar 6, 2021 • edited Loading

zimme commented Mar 6, 2021

zimme commented Mar 6, 2021

kylemh commented Mar 6, 2021 • edited Loading

kylemh commented Mar 7, 2021

codecov-commenter commented Jul 26, 2021

Codecov Report

github-actions bot commented Dec 16, 2022

codecov-io commented Feb 28, 2021 •

edited

Loading

zimme commented Feb 28, 2021 •

edited

Loading

zimme commented Feb 28, 2021 •

edited

Loading

kylemh commented Feb 28, 2021 •

edited

Loading

zimme commented Feb 28, 2021 •

edited

Loading

kylemh commented Mar 6, 2021 •

edited

Loading

kylemh commented Mar 6, 2021 •

edited

Loading