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

Report type-only imports in no-undef-components #1967

Merged
merged 3 commits into from Sep 13, 2022

Conversation

Amorites
Copy link
Contributor

fix #1963

@Amorites
Copy link
Contributor Author

Two issues in CI:

  1. We can't report an error for import { type HelloWorld2 } from './components/HelloWorld2' with @typescript-eslint/parser@4. This is not sound but is acceptable for legacy users. But how to write test here.
  2. Current vuepress config doesn't support optional chaining operator. Optional chaining not working (no, not in the template) vue-loader#1697. I will look into the config with least influence to other codes.(Or when we can migrate directly to vitepress

Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR!

parser: require.resolve('vue-eslint-parser'),
errors: [
{
message: "The '<Foo>' component has been used, but not defined.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to make the message clearer. For example:

'<Foo>' component has been used, but 'Foo' only refers to a type.

What do you think?

@@ -130,7 +130,16 @@ module.exports = {
(scope) => scope.type === 'module'
)
for (const variable of (moduleScope && moduleScope.variables) || []) {
scriptVariableNames.add(variable.name)
const definition = variable.defs[0]
if (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can more easily check if it's a value by doing:

if (!variable.isValueVariable || variable.isValueVariable())

https://github.com/typescript-eslint/typescript-eslint/blob/77d76e21cdc2e100c729c839c292e82ab7c554c5/packages/scope-manager/src/variable/Variable.ts#L24

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment of mine was wrong 😓

@ota-meshi
Copy link
Member

We need to branch using semver regarding test cases failing in tseslint v4. We have already done that in some test cases.

https://github.com/vuejs/eslint-plugin-vue/blob/master/tests/lib/rules/no-export-in-script-setup.js#L54

@@ -1,6 +1,6 @@
{
"compilerOptions": {
"target": "ES2020",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This time I made vuepress support optional chaining operator by lowering the target version.

If it is not proper, please tell me and I will find another way to make it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into what is causing the vuepress build not to work. I didn't know that setting affects vuepress.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I just lost my mind at that moment 😢. This doesn't affect js files, errors just disappear accidentally.

@Amorites
Copy link
Contributor Author

Thanks a lot for your comment, they are extremely helpful.

Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you for your contribution!

Copy link
Member

@FloEdelmann FloEdelmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

tests/lib/rules/no-undef-components.js Outdated Show resolved Hide resolved
@FloEdelmann FloEdelmann changed the title fix(no-undef-components): report error on type-only imports Report type-only imports in no-undef-components Sep 13, 2022
@FloEdelmann FloEdelmann merged commit 6173b91 into vuejs:master Sep 13, 2022
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.

vue/no-undef-components is expected to report error if component is type only
3 participants