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] no-unused-modules: make type imports mark a module as used #1974

Merged
merged 2 commits into from Jan 18, 2021
Merged

[Fix] no-unused-modules: make type imports mark a module as used #1974

merged 2 commits into from Jan 18, 2021

Conversation

cherryblossom000
Copy link
Contributor

@cherryblossom000 cherryblossom000 commented Jan 16, 2021

Fixes #1924

ExportMap now keeps track of each imported declaration (imports.get(path).declarations) and whether the declarations are only importing types (isOnlyImportingTypes). This is used by no-cycle to

  • ignore only type imports
  • report circular dependencies at the correct line

For example (from this existing test):

depth-zero.js

import { bar } from './flow-types-depth-one'

flow-types-depth-one.js

// @flow

// this import declaration doesn't trigger the rule because it's a type import
import type { FooType } from './flow-types-depth-two'
// this declaration does because of the 'bar' import, so the error message refers to this line
import { type BarType, bar } from './flow-types-depth-two'

export { bar }

flow-types-depth-two.js

import { foo } from './depth-one'

depth-one.js

import foo from './depth-zero'

The only significant change in no-unused-modules is here where each declaration's importedSpecifiers is added to a file's import list. However, because the imports in a file's ExportMap now include type imports, type imports now mark an export as being used.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.0%) to 95.917% when pulling 326244f on cherryblossom000:1924 into ff50964 on benmosher:master.

@coveralls
Copy link

coveralls commented Jan 16, 2021

Coverage Status

Coverage increased (+0.003%) to 97.913% when pulling 462b016 on cherryblossom000:1924 into ae5f3ce on benmosher:master.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

If you rebase, many of the styling changes will already be taken care of. I suggest running npx eslint . --fix after the rebase as well.

@@ -49,3 +49,6 @@ lib/
yarn.lock
package-lock.json
npm-shrinkwrap.json

# macOS
.DS_Store
Copy link
Member

Choose a reason for hiding this comment

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

this should be in your global gitignore, rather than adding it to every project you touch (but it’s fine to add)

src/ExportMap.js Outdated Show resolved Hide resolved
src/ExportMap.js Outdated Show resolved Hide resolved
src/ExportMap.js Outdated Show resolved Hide resolved
@saiichihashimoto
Copy link

@ljharb what's the timeline for releases in this repo? I was assuming CI deployment would just take it away but I'm seeing in the CHANGELOG that this is unreleased.

@ljharb
Copy link
Member

ljharb commented Jan 29, 2021

Assuming automatic releases is not a safe assumption; I find that incredibly dangerous and don't do it in any package I maintain.

There is no concrete timeline for releases; I tend to find time every 3-6 months in this project.

@saiichihashimoto
Copy link

saiichihashimoto commented Feb 2, 2021

Can I request that you release soon? It's misleading to mark #1924 (and any other issues with unreleased fixes) as resolved considering it'll continue to be broken for dependents for up to 6 months.

@ljharb
Copy link
Member

ljharb commented Feb 2, 2021

@saiichihashimoto it is a decade-old standard practice that issues are closed when fixes are merged, entirely unrelated to the time of release. If that doesn't match your expectations, I'm afraid the easiest path is to adjust your expectations.

I am indeed planning a release soon, but it's blocked right now due to npm 7's release breaking tests on master.

@saiichihashimoto
Copy link

That makes sense, no sass intended.

@saiichihashimoto
Copy link

What's the progress on this? It's been a while...

@ljharb
Copy link
Member

ljharb commented Apr 14, 2021

It's still blocked on #1986.

@saiichihashimoto
Copy link

Now that #1986 is resolved, is there a timeline of when we can expect a release?

@ljharb
Copy link
Member

ljharb commented May 12, 2021

There is no such timeline. As soon as I move through all the PRs that have been waiting, I plan to cut a release.

@saiichihashimoto
Copy link

Is there a way you could cut a release and then go through those PRs? My understanding is that the changes that are waiting have already been tested so the wait for them to go out is arbitrary.

@ljharb
Copy link
Member

ljharb commented May 12, 2021

Every release that goes out increases maintenance burden for the project, and anything that's not in master yet, but would be in the next release if I wait, might add features or fix bugs, thus reducing that maintenance burden.

@cherryblossom000
Copy link
Contributor Author

@saiichihashimoto If you really want to use eslint-plugin-import with this bug fix without waiting for a release, you could use GitHub dependencies in package.json:

{
  "devDependencies": {
    "eslint-plugin-import": "benmosher/eslint-plugin-import#462b016"
  }
}

462b016 is the commit that merged this PR, but you could also use a newer commit if you want.

@cherryblossom000
Copy link
Contributor Author

@saiichihashimoto v2.23.0 has been released, which includes this bug fix.

@saiichihashimoto
Copy link

Thanks for bearing with my persistence!

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

Successfully merging this pull request may close these issues.

[import/no-unused-modules] False positive while importing TS interfaces
4 participants