Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support TypeScript v4.x #402

Closed
opl- opened this issue Sep 13, 2020 · 23 comments
Closed

Support TypeScript v4.x #402

opl- opened this issue Sep 13, 2020 · 23 comments

Comments

@opl-
Copy link

opl- commented Sep 13, 2020

Versions:

  • prettier-eslint version: 11.0.0

Problem description:

TypeScript has recently released a new update which bumped the major. Unfortunately, TypeScript doesn't adhere to semver (see microsoft/TypeScript#14116). This unfortunately means that if a user's project depends on TypeScript v4.0, npm/yarn will install a separate TypeScript v3.9 just for this package in order to fulfill the dependency version ranges.

Suggested solution:

Using >=3.9.2 as the dependency version for the typescript package should fix this issue while keeping keeping the option to use older, still compatible TypeScript versions. See https://docs.npmjs.com/files/package.json#dependencies.

Relevant: #391

@kylemh
Copy link
Collaborator

kylemh commented Sep 20, 2020

I feel like we could switch this dependency to instead be a peerDependency >=typescript@^3.9.0. What do you think @hamzahamidi ?

The only downside to this would be that, since we're probably not likely to keep track of how each TS version affects this package, we'll eventually have the ceiling reported to us and we'd have to change the range until a fix comes into play. I think this is still better than forcing consumers into multiple versions of TS

@hamzahamidi
Copy link
Collaborator

It's a good idea to give flexibility to the user, but I don't know the effect on the package @typescript-eslint/parser

@kylemh
Copy link
Collaborator

kylemh commented Sep 20, 2020

They follow typescripts release cycle to avoid confusion, so we'd just need to update that dependency as well.

One very interesting question is... where do we stand in regards to SemVer on this?

Should we bump a major if we upgrade typescript and @typescript-eslint/parser 🤔 even weirder, should we be paying attention to every release to follow SemVer?

We could also probably change the way this library works in terms of... maybe we list the key dependencies (eslint, prettier, typescript, @typescript-eslint/parser) as peerDependencies and advertise that we only offer bug support for pairings that our devDependencies are listed as (by the end of Hacktoberfest, that would look like v7, v2, v4, and v4 respectively)?

Could drastically lower the amount of activity in this project's issues/PRs...

@hamzahamidi
Copy link
Collaborator

If we make it a peer dependency. What should we use for the tests ? Should we only use the latest version?
If however we only upgrade typescript & @typescript-eslint/parser to version 4 we should treat it as major version.

@kylemh
Copy link
Collaborator

kylemh commented Sep 20, 2020

@hamzahamidi keep in mind neither of those repositories follow SemVer. There aren't breaking changes in v4.

And yeah, I think latest for our own repo is fine.

@kylemh
Copy link
Collaborator

kylemh commented Sep 23, 2020

Note #133 and the decision zimme reached, but I respectfully disagree, since we have 3 years of other issues to point to as evidence.

@zimme
Copy link
Member

zimme commented Sep 23, 2020

If problems are actually solved by moving prettier amd eslint into peer dependencies I don't have a problem with that.

As we do use prettier and eslint in the closest node_modules from the directory where prettier-eslint is run. Maybe it actually makes sense to have the dependencies as peer dependencies to make sure you have a copy of prettier and eslint there. However Im not sure this will actually solve the problems people are facing as if they want to use another version they can just manually install prettier and eslint in their projects and those versions will be used.

Maybe we shouldnt be using our current approch of resolving prettier and eslint.

The only way to be sure of the best way is probably to try in alpha releases

@kylemh
Copy link
Collaborator

kylemh commented Sep 23, 2020

My guess would be that a large amount of people that use this dependency aren't aware of the ability to use eslint with the prettier plugin to format their code base. There may be some other use-case that I am not aware of, but for those users - if we instruct them to install this via yarn add -D prettier-eslint eslint prettier - there should be no issue.

The bigger hope from me is that this shrugs off this library's responsibility for ensuring the two pair well. Instead, we're just the system of enforcing: lint first, format second.

We'll release its a breaking alpha.

@zimme
Copy link
Member

zimme commented Sep 23, 2020

The reason I think, could be wrong here, the decision was made to make sure the two play nice is that prettier-eslint isnt just providing format first, auto fix second, but it also provides functionality to infer prettier options from eslint configs.

@kylemh
Copy link
Collaborator

kylemh commented Sep 23, 2020

Awesome. I'll write that in the README to explain the differences.

@JonDum
Copy link

JonDum commented Feb 28, 2021

Here's another issue that peerDeps would solve:

https://stackoverflow.com/questions/55807329/why-eslint-throws-no-unused-vars-for-typescript-interface

There's several subtle errors that can't be fixed until prettier-eslint updates its version of @typescript-eslint/parser. In this case, I'm getting no-unused-vars errors because prettier-eslint currently hard pins to @typescript-eslint/pasrer@3.0.0 and there's no way to tell node to use a different version, even if you install @typescript-eslint/parser to a specific version.

npm ls @typescript-eslint/parser
├─┬ @typescript-eslint/eslint-plugin@4.15.2        <--- I have 4.15.2 installed
│ └── @typescript-eslint/parser@4.15.2 deduped
├── @typescript-eslint/parser@4.15.2
├─┬ prettier-eslint-cli@5.0.0
│ └─┬ prettier-eslint@9.0.2
│   └── @typescript-eslint/parser@1.13.0 <--- Yet there's a 1.13.0 here...
└─┬ prettier-eslint@12.0.0
  └── @typescript-eslint/parser@3.10.1 <--- AND a 3.10.1 here...  :(

It would be fine if prettier-eslint team was on top of every release, but historically it hasn't been. I'm not trying to throw blame or anything, just making a point that peerDeps would at least allow users to solve incompatibility users by pinning their own version lest be stuck with the version that prettier-eslint decided.

@kylemh
Copy link
Collaborator

kylemh commented Feb 28, 2021

Couldn't resolutions solve your problem?

@JonDum
Copy link

JonDum commented Feb 28, 2021

@kylemh Relying on a yarn only construct isn't exactly ideal 🤔

@kylemh
Copy link
Collaborator

kylemh commented Feb 28, 2021

https://github.com/rogeriochaves/npm-force-resolutions just trying to give you a workaround for now.

@zimme I haven't used prettier-eslint for some time, but with the advent of prettier/eslint-config-prettier#175 I'm wondering if the library should even exist anymore.

@zimme
Copy link
Member

zimme commented Feb 28, 2021

I don't really use this library any longer as I don't need my prettier setting being inferred from my eslint settings, which is what this library provides over just using prettier && eslint --fix. So there is still a "need" for this package but the recommended way of using prettier with eslint is to actually use eslint-config-prettier which disables all eslint rules that conflict with prettier.

@zimme
Copy link
Member

zimme commented Feb 28, 2021

I think it would be a good idea to update the readme with a recommendation to use prettier, eslint and eslint-config-prettier directly unless you want the prettier config to be changed based on your eslint config.

@zimme
Copy link
Member

zimme commented Feb 28, 2021

I guess there's also a use case for this lib in editors which don't support multiple linter/formatters on one file type

@zimme
Copy link
Member

zimme commented Feb 28, 2021

As for this specific issue. I do believe we walt to changed all the typescript/vue/other parsers to optional dependencies and check if thery're available and only use them in those cases.

As well as loosening the version ranges on those dependencies.

@kylemh
Copy link
Collaborator

kylemh commented Feb 28, 2021

It didn't seem like optionalDependencies was what we wanted.

https://docs.npmjs.com/cli/v7/configuring-npm/package-json#optionaldependencies

Entries in optionalDependencies will override entries of the same name in dependencies, so it's usually best to only put in one place.


Gave it a whirl though: #505 👀

@JonDum
Copy link

JonDum commented Feb 28, 2021

Quick follow up: I switched to yarn and tried resolutions:

[1/4] 🔍  Resolving packages...
warning Resolution field "@typescript-eslint/parser@4.15.2" is incompatible with requested version "@typescript-eslint/parser@^3.0.0"
warning Resolution field "@typescript-eslint/parser@4.15.2" is incompatible with requested version "@typescript-eslint/parser@^1.10.2"

So that's a no go. I think @kylemh's #505 is the way to go moving forward.


Or, is this library just not recommended to be used anymore? Should it be deprecated in favor of an eslint --fix route?

@kylemh
Copy link
Collaborator

kylemh commented Feb 28, 2021

@JonDum the warnings are expected, but you should still end up getting the latest version. You can verify by looking at the resolved version in the lockfile.

Also, see my README edits in #505... See if that helps you make a decision re: this or eslint-config-prettier.

@idahogurl
Copy link
Collaborator

I guess there's also a use case for this lib in editors which don't support multiple linter/formatters on one file type

I use this library for this reason. I have a Visual Studio Code plugin I wrote before the Prettier extension supported ESLint formatting. Plus people have flocked to mine because it's easier to setup.

@JounQin
Copy link
Member

JounQin commented Aug 15, 2022

close in favor of #696

@JounQin JounQin closed this as completed Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants