search

prettier / prettier-eslint-cli

CLI for prettier-eslint
https://npm.im/prettier-eslint-cli
MIT License
539 stars 85 forks source link

WIP: update eslintrc for validation testing #435

Closed mattbrannon closed 2 years ago

mattbrannon commented 2 years ago

I recently forked this project and wanted to try tackling some issues. However, I ran into problems with the validation script npm start validate. The first set of tests ran normally but the second set, which internally runs nps test.cli failed in 4 out of 7 tests with the error message Environment key \"jest/globals\" is unknown which the package eslint-config-kentcdodds uses in it's configuration.

Screen Shot 2022-07-31 at 8 48 42 AM

Removing the extends field and adding @babel/eslint-parser as a parser allows the tests to run normally.

Specifically this PR does the following:

Please let me know if there's anything that needs to change. This is quick fix that allows the tests to run to completion without blowing up halfway through. I assume that Kent knows what he's doing in terms of his eslint + jest configuration but, at the moment, it appears to be causing an issue. It's unclear to me exactly where in the chain the problem occurs.

JounQin commented 2 years ago

I'm not sure why don't you post issue or fixing PR on https://github.com/kentcdodds/eslint-config-kentcdodds first? An ESLint config package is much more than a parser.

cc @kentcdodds

mattbrannon commented 2 years ago

I'm not sure why don't you post issue or fixing PR on https://github.com/kentcdodds/eslint-config-kentcdodds first? An ESLint config package is much more than a parser.

I understand that, hence the WIP title. I haven't posted an issue on the other repo because it's not clear to me the underlying cause at this point.

JounQin commented 2 years ago

I understand that, hence the WIP title.

You should mark this PR as draft then, I've changed it for you.

mattbrannon commented 2 years ago

I understand that, hence the WIP title.

You should mark this PR as draft then, I've changed it for you.

Ah, yes I see. Thank you.

JounQin commented 2 years ago

I believe your issue if caused by @rushstack/eslint-patch which is used by eslint-config-next.

https://github.com/prettier/prettier-eslint-cli/issues/434#issuecomment-1207331852

mattbrannon commented 2 years ago

@JounQin I'm not sure I understand. #434 Is the reason I started digging into the codebase but is otherwise unrelated I believe. Maybe I should have been more specific about the goal here. I submitted this PR to try and have a discussion about possible solutions to validation tests failing during the initial setup process. Specifically step 3 of the contributing guide which says,

$ npm start validate to validate you've got it working

Running npm start validate causes the errors mentioned above. While the errors may indeed be due to a conflict with @rushstack/eslint-patch it seems odd to pass this off as a problem on their end.

JounQin commented 2 years ago

OK, I may misunderstood what you were trying to resolve, but what you're trying to change in this PR is definitely not a solution for what you want to fix. They are just bypassing the expected checks.

I'd be happy if you provide a more specific PR for fixing your mentioned issue. Feel free to create a new PR. Thanks.

JounQin commented 2 years ago

I can reproduce it on CI now

https://github.com/prettier/prettier-eslint-cli/runs/7797880791?check_suite_focus=true#step:5:16

If you have a good solution to fix this, feel free to share!

Fixed at 46f2a6e (#437)

mattbrannon commented 2 years ago

@JounQin I cloned your branch and installed locally on my machine. I ran the validate script without any errors this time. Nice work. Thanks for putting in the time on this.