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: Separate ESLint API options from option.eslintConfig #798

Closed

Conversation

qyurila
Copy link

@qyurila qyurila commented Oct 9, 2022

This PR fixes the following error that occurs when reportUnusedDisableDirectives option in ESLint config file is set to true or false:

Error: Invalid Options:
- 'reportUnusedDisableDirectives' must be any of "error", "warn", "off", and null. 

The property reportUnusedDisableDirectives should be boolean. What must be "error", "warn", "off", or null is one of API options which has the identical name.

Relevant PR: #735

@changeset-bot
Copy link

changeset-bot bot commented Oct 9, 2022

⚠️ No Changeset found

Latest commit: 0cb51b2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@qyurila qyurila force-pushed the fix/eslint-invalid-options-error branch from 6e0121d to 63e22e8 Compare October 9, 2022 21:32
@qyurila qyurila changed the title fix: Parse reportUnusedDisableDirectives option of ESLint config correctly refactor: Separate ESLint API options from option.eslintConfig Oct 9, 2022
@qyurila
Copy link
Author

qyurila commented Oct 9, 2022

Well, this PR ended up becoming kinda breaking change so I changed the title.

Previously, the ESLint API options, which should be served to ESLint constructor as a parameter (ESLintOptions, contains e.g. fix), were constructed in options.eslintConfig, which is reserved for ESLint override config (contains e.g. rules), and poped later. This was the underlying cause that made reportUnusedDisableDirectives config option to be transferred to new ESLint() as an API option.

This PR fixes the problem by creating options.eslintConfig.apiOptions property and migrating all API options inside it. It's technically a breaking change, but it would be barely a problem because the API options thing had been not documented nor intended to use (I hope). Therefore there are a lot of tests updated but I believe it would not affect the current usage of prettier-eslint.

@qyurila qyurila marked this pull request as ready for review October 9, 2022 22:14
@qyurila
Copy link
Author

qyurila commented Oct 9, 2022

Note: the half of diffs were caused by all-contributors update: all-contributors/cli#307.

@codecov-commenter
Copy link

codecov-commenter commented Oct 10, 2022

Codecov Report

Base: 99.66% // Head: 99.65% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (63e22e8) compared to base (ffe2c45).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 63e22e8 differs from pull request most recent head 0cb51b2. Consider uploading reports for the commit 0cb51b2 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #798      +/-   ##
==========================================
- Coverage   99.66%   99.65%   -0.01%     
==========================================
  Files           2        2              
  Lines         298      292       -6     
  Branches       88       88              
==========================================
- Hits          297      291       -6     
  Misses          1        1              
Impacted Files Coverage Δ
src/index.js 99.15% <100.00%> (-0.05%) ⬇️
src/utils.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@qyurila qyurila changed the title refactor: Separate ESLint API options from option.eslintConfig fix: Separate ESLint API options from option.eslintConfig Oct 10, 2022
The handling of `options.eslintConfig` was a bit of a mess. Config
properties and API options had been merged into the same object and
manually divided one by one.
Even though this is technecally a breaking change, it would be barely a
problem because the moved part had been not documented nor was
intended to use.

BREAKING CHANGE: `options.eslintConfig` no longer accepts ESLint
API options (e.g. `extensions`, allowInlineConfig), despite they were
not officially supported regarding to document. From now they
must be in `options.eslintConfig.apiOptions`.
@qyurila qyurila force-pushed the fix/eslint-invalid-options-error branch from 63e22e8 to 546b1e2 Compare October 11, 2022 07:56
The huge diff is caused by all-contributors/cli update.
@github-actions
Copy link
Contributor

Stale pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants