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

Rule Proposal: disallow imports from @vue/* dependencies #1780

Closed
jbhammon opened this issue Jan 25, 2022 · 10 comments · Fixed by #1804
Closed

Rule Proposal: disallow imports from @vue/* dependencies #1780

jbhammon opened this issue Jan 25, 2022 · 10 comments · Fixed by #1804

Comments

@jbhammon
Copy link

Please describe what the rule should do:

Disallow or warn about imports from @vue/* dependencies. Particularly when using Vue CLI and the build target is a library.

What category should the rule belong to?

[ ] Enforces code style (layout)
[x] Warns about a potential error (problem)
[ ] Suggests an alternate way of doing something (suggestion)
[ ] Other (please specify:)

Provide 2-3 code examples that this rule should warn about:

<!-- Fail or Warn -->
import { computed } from "@vue/reactivity";

import {
  something,
  another,
} from "@vue/compiler-dom";
<!-- Pass -->
import { computed } from "vue";

Additional context

Full context can be found in this issue I opened in the core repo.

In short, using the @vue/reactivity import broke the components I was building for my UI library with Vue CLI and they were failing silently.

@ota-meshi
Copy link
Member

Thank you for posting this issue.
I think you can do that using the rule of the ESLint core.
https://eslint.org/docs/rules/no-restricted-imports

@jbhammon
Copy link
Author

Very good point, thanks for linking the resource. I'll try putting a rule together using that in the coming days!

@FloEdelmann
Copy link
Member

Note that many people are (slowly) migrating from Vue 2 to Vue 3 and start by using the Composition API plugin like that:

import { ref, reactive } from '@vue/composition-api'

@jbhammon
Copy link
Author

Good to know, thanks. I can see about excluding that specific plugin in the rule, unless there are others you know of to avoid?

This is also the first time I'm working with eslint, so I'm not sure of its limitations. The rule I suggested here is intended to help prevent people from bundling Vue when they're building a library with Vue CLI (with the --target lib flag). If they're building a regular Vue app these sorts of imports are (probably?) fine. Is there a way to apply this rule only when a particular command like that is run, or are all the rules always applied each time code is linted?

@FloEdelmann
Copy link
Member

These are some packages in the @vue scope that I found:

List
  • @vue/babel-plugin-jsx
  • @vue/cli
  • @vue/cli-plugin-e2e-cypress
  • @vue/cli-plugin-e2e-nightwatch
  • @vue/cli-plugin-unit-jest
  • @vue/cli-service
  • @vue/compat
  • @vue/compiler-core
  • @vue/compiler-dom
  • @vue/compiler-sfc
  • @vue/compiler-ssr
  • @vue/component-compiler-utils
  • @vue/composition-api
  • @vue/eslint-config-typescript
  • @vue/reactivity
  • @vue/reactivity-transform
  • @vue/runtime-core
  • @vue/runtime-dom
  • @vue/runtime-test
  • @vue/server-renderer
  • @vue/shared
  • @vue/test-utils
  • @vue/web-component-wrapper

I haven't checked which ones are allowed to be imported from libraries and which aren't. If you are planning to write a new rule for this ESLint plugin, I'd rather only report specific packages, so we don't necessarily have to keep up with new ones.

I don't think there is any way of knowing whether the project is built as a library. If the rule is not included in any preset, users will have to explicitly enable it on their own, so that should be okay.

@LinusBorg
Copy link
Member

LinusBorg commented Jan 30, 2022

Thank you for posting this issue. I think you can do that using the rule of the ESLint core. eslint.org/docs/rules/no-restricted-imports

@ota-meshi can we wrap eslint's no-restricted-imports to allow us to make this part of one of our presets?

The rule could look something like this:

'no-restricted-imports': [
  'error',
  {
    paths: ['@vue/reactivity', '@vue/runtime-core', '@vue/runtime-dom', '@vue/shared'],
    message: "Please always import these APIs from the main 'vue' package"
  }
]

Excluding these runtime packages should be enough to catch most of the problematic autocomplete imports like import { ref } from '@vue/reactivity'

@ota-meshi
Copy link
Member

If importing @vue/* is a bad practice for everyone, I think it's good to add a new rule. Perhaps the rule can support auto-fix.
I think it would be nice to add that rule to a preset, but I'm not sure which preset is best to add the rule to.
What do you think?

@LinusBorg
Copy link
Member

LinusBorg commented Feb 3, 2022

At the very least, importing from the following packages is practically always a mistake, usually by a bad autocomplete suggestion in the editor/IDE:

  • @vue/reactivity
  • @vue/runtime-core
  • @vue/runtime-dom
  • @vue/shared

These are transitive dependencies of vue and should not be accessed directly. This works with package managers like npm or yarn 1 which hoist all dependencies to node_modules, but fails with package managers that are stricter, like pnpm.

So someone switching from npm to pnpm could find their app to be broken in lots if places because imports from those packages don't work anymore.

For library authors it's even worse: Excluding 'vue' from the bundle is recommended best practice, but that would not exclude direct imports from these transitive dependencies. That means they get included in the bundle, increasing bundle size at best and breaking the library at worst, since Vue relies on a couple of singleton values for reactivity and scheduling.

So yeah, restricting imports from these transitive dependencies would benefit all users, and since it's not just a cosmetic thing, I would tend to add it to essentials.

@jbhammon
Copy link
Author

jbhammon commented Feb 5, 2022

I just opened a PR to try adding what Linus outlined above. My tests currently aren't passing so I don't think I've implemented it completely correct. @ota-meshi or @FloEdelmann I'll be thankful for any help to get this working.

@ota-meshi
Copy link
Member

@jbhammon Thank you for the pull request.

However, I don't think the new rule is an extended rule for no-restricted-imports.

The rule reports an import from @vue/* that can be replaced with an import from "vue" and will fix it automatically if possible.
It is not possible to set the path that the user is free to report like no-restricted-imports.
I think the new rule will have to be implemented on its own, without using the core rule extension.

Also, the rule name will probably be different.
For example, I think it would be the name of new rule like prefer-import-from-vue or no-import-from-at-vue.

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