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

[New] no-unused-modules: support dynamic imports #1660

Merged

Conversation

maxkomarychev
Copy link
Contributor

@maxkomarychev maxkomarychev commented Feb 17, 2020

All occurences of import('...') are treated as namespace imports
(import * as X from '...')

Fixes #1951.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 97.693% when pulling 060ea2c on maxkomarychev:no-unused-modules-dynamic-imports into 12971f5 on benmosher:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 97.693% when pulling 060ea2c on maxkomarychev:no-unused-modules-dynamic-imports into 12971f5 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 97.693% when pulling 060ea2c on maxkomarychev:no-unused-modules-dynamic-imports into 12971f5 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 97.693% when pulling 060ea2c on maxkomarychev:no-unused-modules-dynamic-imports into 12971f5 on benmosher:master.

@coveralls
Copy link

coveralls commented Feb 17, 2020

Coverage Status

Coverage decreased (-5.07%) to 92.784% when pulling c10e217 on maxkomarychev:no-unused-modules-dynamic-imports into 36a535b on benmosher:master.

@ljharb
Copy link
Member

ljharb commented Feb 17, 2020

const a = await import('a') is not equivalent to import a from 'a', it's equivalent to getting a promise for import * as mod from 'a'. You might be thinking of const { default: a } = await import('a')

await is largely irrelevant here, i think.

Because import() produces a promise, I don't think there's any value in checking that specific imports/exports are used or not, because it's not very reliably to detect that.

@maxkomarychev
Copy link
Contributor Author

hey, thanks for feedback!

const a = await import('a') is not equivalent to import a from 'a', it's equivalent to getting a promise for import * as mod from 'a'. You might be thinking of const { default: a } = await import('a')

good point, you're right - I should be treating this as namespace :)

await is largely irrelevant here, i think.
Because import() produces a promise, I don't think there's any value in checking that specific imports/exports are used or not, because it's not very reliably to detect that.

Not sure I entirely follow here. I was not going to check if any specific name imported from the module has ever been referenced (there are other eslint rules for this matter) but when import is assigned to destructured object I want to mark specific names that were imported, isn't this good for this rule?

@ljharb
Copy link
Member

ljharb commented Feb 18, 2020

Right, that's what I'm saying - it's not going to be safe in enough cases to mark specific names, so any path that's dynamically imported should be treated as potentially using every export.

@maxkomarychev
Copy link
Contributor Author

maxkomarychev commented Feb 22, 2020

Right, that's what I'm saying - it's not going to be safe in enough cases to mark specific names, so any path that's dynamically imported should be treated as potentially using every export.

Even though handling all kinds of expressions would be very complex I believe it is possible to handle some finite amount of patterns and it would increase accuracy of the lib, e.g.

import base with await with then
import * as a from 'a' const a = await import('a') import('a').then(a =>{}) or import('a').then(function(a){})
import { a } from 'a' const { a } = await import('a') import('a').then(({a}) =>{}) or import('a').then(function(({a}){})

all other calls to import('a') will simply be treated as namespace import

is that something you would consider to review?

updated version of table

yeah I figured out all calls to import not assigned to object pattern should be treated as namespace imports, so it results in less specific cases:

import base with await with then
import { a } from 'a' const { a } = await import('a') import('a').then(({a}) =>{}) or import('a').then(function(({a}){})

all other calls to import('a') will simply be treated as namespace import

@ljharb
Copy link
Member

ljharb commented Feb 22, 2020

In your first row, the a could be passed elsewhere. I'm not sure how often the second one happens to be useful.

import() is also often used behind an abstraction, or in Promise.all like Promise.all([import('a'), import('b')]), which is then either awaited or .thend. Do you propose attempting to handle those cases as well?

@maxkomarychev
Copy link
Contributor Author

alright, this is too much :) I'll treat import('a') as namespace no matter what

@maxkomarychev
Copy link
Contributor Author

maxkomarychev commented Feb 25, 2020

@ljharb I almost cleaned it up although I am stuck at typescript files - I put a dedicated test with typescript parser and it is always green - how do I configure it to work properly - it looks like it is simply skipping ts file.

@maxkomarychev maxkomarychev force-pushed the no-unused-modules-dynamic-imports branch from 2ae6d70 to db319e3 Compare March 10, 2020 18:44
@maxkomarychev
Copy link
Contributor Author

ok I believe I figured this out, although one last bit that puzzles me - dynamic imports are supported by espree since 6.1.0 (eslint/espree@f5e58cc) but tests fail even if ecmaVersion is set to 11. so I can only got it working with babel-eslint or @typescript-eslint/parser. any suggestion how to make espree to handle this? or would it instead be acceptable to just specify this in docs for specific rule to use custom parser?

@ljharb
Copy link
Member

ljharb commented Mar 10, 2020

It wasn't stage 4 until ES2020, so eslint wouldn't support it until that ecmaVersion.

If it's still not working with the right ecmaVersion, it might not work in eslint until v7; for now, it's fine just to support those two parsers.

@maxkomarychev maxkomarychev marked this pull request as ready for review March 10, 2020 18:54
@maxkomarychev
Copy link
Contributor Author

maxkomarychev commented Mar 10, 2020

should I modify readme with this?
otherwise - ready for review 🎆
thanks

@maxkomarychev maxkomarychev force-pushed the no-unused-modules-dynamic-imports branch 3 times, most recently from 533df7f to 4e2e78c Compare March 11, 2020 07:24
@maxkomarychev
Copy link
Contributor Author

it fails tests on eslint 2 and 3, what's the best way to deal with these?

@maxkomarychev
Copy link
Contributor Author

bump?

@maxkomarychev maxkomarychev force-pushed the no-unused-modules-dynamic-imports branch 2 times, most recently from 689264a to cff6dbb Compare April 17, 2020 20:03
src/ExportMap.js Show resolved Hide resolved
src/ExportMap.js Outdated Show resolved Hide resolved
@Hypnosphi
Copy link
Contributor

Hypnosphi commented May 20, 2020

@maxkomarychev looks like you also have to update https://github.com/benmosher/eslint-plugin-import/blob/master/src/rules/no-unused-modules.js#L610 to include dynamic imports into newNamespaceImports

@maxkomarychev
Copy link
Contributor Author

@Hypnosphi can you elaborate please? this pr puts dynamic imports with as they are namespace imports, given that all other code is using ExportMap then it should be transparent to them with no special handling, unless I'm missing something.

Thanks.

@aladdin-add
Copy link
Contributor

friendly ping @ljharb

@ljharb
Copy link
Member

ljharb commented Aug 13, 2021

I'll take a look at this in the next 24 hours or so.

@ljharb ljharb force-pushed the no-unused-modules-dynamic-imports branch 2 times, most recently from eee0cc8 to 4f338bf Compare August 14, 2021 05:26
@ljharb ljharb changed the title no-unused-modules: support dynamic imports [New] no-unused-modules: support dynamic imports Aug 14, 2021
@ljharb ljharb marked this pull request as ready for review August 14, 2021 05:33
@ljharb
Copy link
Member

ljharb commented Aug 14, 2021

@Hypnosphi perhaps i messed up the rebase, but a bunch of tests are still failing. could you make sure to fetch the latest branch, and then get the tests passing?

@ljharb ljharb marked this pull request as draft August 14, 2021 05:37
@Hypnosphi
Copy link
Contributor

Sorry, it's hard to track what's changed here comparing to my branch. I think I'll open a separate PR

@ljharb
Copy link
Member

ljharb commented Aug 30, 2021

@Hypnosphi i really, really wish you had not done that (see #1660 (comment) where i explicitly asked you not to); PRs once opened are eternally present in a repo's refs. Now I just have to keep both of these PRs in sync until they're landed.

@ljharb ljharb force-pushed the no-unused-modules-dynamic-imports branch 2 times, most recently from 63c3c5a to b724db8 Compare August 30, 2021 17:40
ljharb pushed a commit to ljharb/eslint-plugin-import that referenced this pull request Aug 30, 2021
ljharb pushed a commit to ljharb/eslint-plugin-import that referenced this pull request Aug 30, 2021
ljharb pushed a commit to Hypnosphi/eslint-plugin-import that referenced this pull request Sep 14, 2021
See import-js#1660, import-js#2212.

Co-authored-by: Max Komarychev <maxkomarychev@gmail.com>
Co-authored-by: Filipp Riabchun <filipp.riabchun@jetbrains.com>
Co-authored-by: 薛定谔的猫 <weiran.zsd@outlook.com>
ljharb pushed a commit to Hypnosphi/eslint-plugin-import that referenced this pull request Sep 14, 2021
All occurences of `import('...')` are treated as namespace imports
(`import * as X from '...'`)

See import-js#1660, import-js#2212.

Co-authored-by: Max Komarychev <maxkomarychev@gmail.com>
Co-authored-by: Filipp Riabchun <filipp.riabchun@jetbrains.com>
Co-authored-by: 薛定谔的猫 <weiran.zsd@outlook.com>
@ljharb ljharb force-pushed the no-unused-modules-dynamic-imports branch from b724db8 to b7bf750 Compare September 14, 2021 23:30
@Evertt
Copy link

Evertt commented Sep 8, 2022

Hey, so I'm using this plugin at version 2.26.0, which I believe should include this merged PR already, right? Well I still get a false positive with this.

image

image

So I'm guessing there's still a little bug left in this feature? Can I do anything to help you track down the bug?

@aladdin-add
Copy link
Contributor

yes, please file a new issue if you still encounter it.

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.

[no-unused-modules] Dynamic import doesn't mark exports as used
6 participants