-
Notifications
You must be signed in to change notification settings - Fork 22
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
#570: Extract and deduplicate webpack resolution config #761
Conversation
.eslintrc
Outdated
|
||
// TODO: Restore these after https://github.com/benmosher/eslint-plugin-import/issues/1931 | ||
// TODO: Restore this after linting the codebase | ||
// "plugin:import/errors", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR does not attempt to lint the codebase, so I can't enable this preset yet because it will fail the build. Also it's just possible that it's because YAML isn't working with eslint even without the shared configuration. Example:
24:3 error Schema not found in '@cfworker/json-schema' import/named
25:3 error ValidationResult not found in '@cfworker/json-schema' import/named
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that it's because YAML isn't working with eslint
The @cfworker/json-schema
package is written in Typescript: https://github.com/cfworker/cfworker/tree/main/packages/json-schema/src
Not sure what you mean by YAML isn't working?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YAML isn't working with eslint
I mean that import './file.yaml'
has a behavior defined by a webpack loader, it means nothing in node/JS, and my guess was that those imports were causing errors with this plugin for this reason.
However the errors were because the import/named
rule doesn't understand type imports. Schema
is just a type and it cannot find it. Since these are handled by TypeScript, I now disabled the rule.
I ended up fixing the one remaining issue so import/errors
is now fully enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the comment back to avoid future confusion: #761 (comment)
@fregante Approved - go ahead an merge when ready |
That's a charming way to say ":shipit:" |
This was low priority but the solution was pretty simple:
In the future:
Side note: The plugin is quite buggy in general (350 open issues for a single eslint plugin) so we might not always be able to follow its suggestions.