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

[FIX] Don't overwrite parser if provided by user #197

Closed
wants to merge 4 commits into from
Closed

[FIX] Don't overwrite parser if provided by user #197

wants to merge 4 commits into from

Conversation

bradzacher
Copy link
Contributor

@bradzacher bradzacher commented Jan 9, 2019

Bug Repro Gist: https://gist.github.com/bradzacher/07d98abce83ed5a4094ccbfacca8b82f

prettier-eslint overrides the user provided parser with its own parser if it detects it's running on a typescript file.

This was generally okay, because there was very little difference between the parser versions (though there may have been subtle bugs that users didn't notice).

However, I was testing a config with the current 1.1.0 of @typescript-eslint/eslint-plugin, and noticed that it wasn't fixing an error I know has a fixer. When I ran eslint --fix manually, it worked fine.

After a lot of digging, I finally setup the repro case and noticed that the config passed into eslint included prettier-eslint's local parser (typescript-eslint-parser), instead of my configured parser (@typescript-eslint/parser).

This change fixes this by allowing the user specified parser to be used.

@bradzacher
Copy link
Contributor Author

just pinging on this.
Is there an expected release window for this?
Right now I'm manually patching my node_modules folders and it's kind of clunky

@j-f1
Copy link
Member

j-f1 commented Feb 8, 2019

(OT: @bradzacher check out patch-package)

@bradzacher
Copy link
Contributor Author

My bigger problem is that the vscode plugin is broken because of this.

prettier-vscode uses its local version of prettier-eslint (ie. the broken one). So I have to not only patch my local prettier-eslint package so my yarn format command works, but also the vscode extension's package so my formatOnSave works.

It's annoying to keep track of when the vscode extension's packages get reinstalled (i have no idea when it happens - i assume when the extension updates?).

@zimme
Copy link
Member

zimme commented Feb 13, 2019

I appreciate the PR and I've been looking a bit at this project today, but I don't see this being merged until we/I can fix the failing tests situation.

There's been updates to prettier to use a new version of cosmiconfig which reads config files differently than before and because of this the filesystem mocking we do in this project isn't working correctly any longer and that's quite a big change to fix. I have, however, done something similar in my linter project (here) which I hope I can re-use here... but I don't have any estimation on when I will have the time to do this

@chinesedfan
Copy link
Contributor

@zimme With the CI fixed, can you re-run the build to check this PR?

@bradzacher By the way, I suggest apply similar changes for vue parser.

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

@zimme
Copy link
Member

zimme commented Jun 12, 2019

I'll rebase this PR tonight if I have time and apply similar changes for vue too.

Bug Repro Gist: https://gist.github.com/bradzacher/07d98abce83ed5a4094ccbfacca8b82f

prettier-eslint overrides the parser with its own parser if it detects it's running on a typescript file.

This was generally okay, because there was very little difference between the parser versions (though there may have been subtle bugs that users didn't notice).

I was testing a config with the current `1.0.0-rc.2` of `eslint-plugin-typescript`, and noticed that it wasn't fixing an error I know has a fixer. When I ran `eslint --fix` manually, it worked fine.

After a lot of digging, I finally setup the repro case and noticed that the config passed into eslint included prettier-eslint's local dependency, instead of eslint-plugin-typescript's.

This matters now for two reasons:
1) As of eslint-plugin-typescript 1.0.0 ([1.0.0 release notes](https://github.com/bradzacher/eslint-plugin-typescript/releases/tag/1.0.0-rc.0)), we've moved the parser internally so we can better manage the dependency (too many bugs reported from people using an unsupported version of the parser).
2) The parser recently released a number of very breaking changes (I think it was the v20 release).

This change fixes this by allowing the user specified parser to be used.

While I haven't tested, I suspect that prettier-eslint might break if used on typescript vue files (if it even works with it?? I don't use vue...)
eslint-plugin-vue uses its own parser as well, which would be clobbered by this setting.
@codecov-io
Copy link

codecov-io commented Jun 12, 2019

Codecov Report

Merging #197 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #197   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines         208    208           
  Branches       42     42           
=====================================
  Hits          208    208
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️

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 2433f68...951ad54. Read the comment docs.

@bradzacher
Copy link
Contributor Author

yay for the CI being fixed!
I rebased the branch, added similar handling to vue, and upgraded the typescript parser to @typescript-eslint/parser.

package.json Outdated Show resolved Hide resolved
@bradzacher
Copy link
Contributor Author

Merged in 668282f and b73fef1

@bradzacher bradzacher closed this Jul 3, 2019
@bradzacher bradzacher deleted the patch-1 branch July 3, 2019 15:38
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

Successfully merging this pull request may close these issues.

None yet

5 participants