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

[6.0.0-alpha.1] RuleTester fails tests if parser property is a package name #11728

Closed
mysticatea opened this issue May 16, 2019 · 5 comments
Closed

Comments

@mysticatea
Copy link
Member

@mysticatea mysticatea commented May 16, 2019

From import-js/eslint-plugin-import#1362
From https://travis-ci.org/benmosher/eslint-plugin-import/jobs/533124402

Tell us about your environment

  • ESLint Version: 6.0.0-alpha.1
  • Node Version: 10.15.3
  • npm Version: 6.4.1

What parser (default, Babel-ESLint, etc.) are you using?

n/a

Please show your full configuration:

n/a

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

Use RuleTester with parser option with babel-eslint. (e.g., eslint-plugin-import:/tests/src/rules/default.js#L31)

What did you expect to happen?

No errors.

What actually happened? Please include the actual, raw output from ESLint.

The test failed with "Parsers provided as strings to RuleTester must be absolute paths."

Are you willing to submit a pull request to fix this bug?

Yes.


I think that RuleTester should allow package names (maybe it loads it relative to CWD).

Also, the Migration Guide must describe this change.

@mysticatea mysticatea added this to Needs discussion in v6.0.0 May 16, 2019
mysticatea added a commit that referenced this issue May 16, 2019
@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented May 16, 2019

The current behavior is intentional because it's not clear where packages should be loaded from with RuleTester. I suppose the CWD works, but it could be confusing because projects using RuleTester might be dependencies of the user's project, so they can't guarantee that their dependencies will be loadable from the CWD. It seems like it would be safer to require an absolute path so that users use require.resolve to avoid the ambiguity.

@platinumazure
Copy link
Member

@platinumazure platinumazure commented May 16, 2019

I think I agree with @not-an-aardvark. We should just document this as a breaking change and require users to specify a full path.

@mysticatea
Copy link
Member Author

@mysticatea mysticatea commented May 17, 2019

Thank you for the explanation.

But, my position is different. Because to use file paths for parser config is a pretty rare case (probably only we are). For 99.99% of users, parser is packages such as babel-eslint, vue-eslint-parser, @typescript-eslint/parser. and etc. Not a file path. Therefore, disallowing a package name would be a surprising behavior.

I think that good design is "people can do the most popular use case easily, and can do rare cases with detailed way." I believe we should allow a package name and show a understandable error message for rare cases (E.g., '${parser}' was not found on '${cwd}'. Make sure the package to be installed or use an absolute path with 'require.resolve()'.). The current behavior seems to enforce annoying to users by afraid of pretty rare cases.

@mysticatea
Copy link
Member Author

@mysticatea mysticatea commented May 21, 2019

TSC Summary:

This is an awareness that came from the failing CI build of eslint-plugin-import. We no longer support a package name in the parser option in RuleTester. This has been introduced in eslint/rfcs#7 intentionally because where it's unclear to load the package from. But for 99.99% of users, parser is packages such as babel-eslint, vue-eslint-parser, @typescript-eslint/parser. and etc. Therefore, disallowing a package name would be a surprising behavior.

Opinions:

TSC Question:

What is the direction we want to go?

  1. The parser requires an absolute path. Just we add the description of that into the migration guide.
  2. The parser allows a package name as similar to ESLint 5 and loads it from CWD. We add the change of the place that ESLint loads the package from, into the migration guide. (On ESLint 5, RuleTester had loaded packages from the location ESLint was installed.)
  3. Others.

@mysticatea
Copy link
Member Author

@mysticatea mysticatea commented May 23, 2019

TSC Resolution: we won't change the current behavior, the parser option requires an absolute path. We will add it to the migration guide.

@mysticatea mysticatea moved this from Needs discussion to Accepted, ready to implement in v6.0.0 May 23, 2019
@platinumazure platinumazure changed the title [6.0.0-alpha.1] RuleTester fails tests if parser proeprty is a package name [6.0.0-alpha.1] RuleTester fails tests if parser property is a package name May 23, 2019
mysticatea added a commit that referenced this issue May 24, 2019
@mysticatea mysticatea moved this from Accepted, ready to implement to Implemented, pending review in v6.0.0 May 24, 2019
v6.0.0 automation moved this from Implemented, pending review to Done May 25, 2019
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Nov 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
v6.0.0
  
Done
3 participants