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

@coveralls coveralls commented Feb 17, 2020

Coverage Status

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

Loading

3 similar comments
@coveralls
Copy link

@coveralls coveralls commented Feb 17, 2020

Coverage Status

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

Loading

@coveralls
Copy link

@coveralls coveralls commented Feb 17, 2020

Coverage Status

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

Loading

@coveralls
Copy link

@coveralls coveralls commented Feb 17, 2020

Coverage Status

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

Loading

@coveralls
Copy link

@coveralls 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.

Loading

@ljharb
Copy link
Collaborator

@ljharb 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.

Loading

@maxkomarychev
Copy link
Contributor Author

@maxkomarychev maxkomarychev commented Feb 18, 2020

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?

Loading

@ljharb
Copy link
Collaborator

@ljharb 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.

Loading

@maxkomarychev
Copy link
Contributor Author

@maxkomarychev 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

Loading

@ljharb
Copy link
Collaborator

@ljharb 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?

Loading

@maxkomarychev
Copy link
Contributor Author

@maxkomarychev maxkomarychev commented Feb 23, 2020

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

Loading

tests/src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
Loading
@maxkomarychev
Copy link
Contributor Author

@maxkomarychev 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.

Loading

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

@maxkomarychev maxkomarychev commented Mar 10, 2020

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?

Loading

@ljharb
Copy link
Collaborator

@ljharb 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.

Loading

@maxkomarychev maxkomarychev marked this pull request as ready for review Mar 10, 2020
@maxkomarychev
Copy link
Contributor Author

@maxkomarychev maxkomarychev commented Mar 10, 2020

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

Loading

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

@maxkomarychev maxkomarychev commented Mar 11, 2020

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

Loading

@maxkomarychev maxkomarychev requested a review from ljharb Mar 30, 2020
@maxkomarychev
Copy link
Contributor Author

@maxkomarychev maxkomarychev commented Apr 10, 2020

bump?

Loading

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

@Hypnosphi 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

Loading

@maxkomarychev
Copy link
Contributor Author

@maxkomarychev maxkomarychev commented May 21, 2020

@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.

Loading

@Hypnosphi
Copy link
Contributor

@Hypnosphi Hypnosphi commented Aug 9, 2021

@ljharb I tried adding export * from '@org/package' to various test files but still wasn't able to reproduce the issue reported by @aladdin-add

Loading

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Aug 9, 2021

@aladdin-add any chance you could come up with a failing test case?

Loading

@aladdin-add
Copy link
Contributor

@aladdin-add aladdin-add commented Aug 10, 2021

I was not able to repro it either - it happened in a large and private repo. /_ \

it seems a very edge case. we can try to fix it when someone else hit the issue?

Loading

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Aug 10, 2021

Sure! This PR then would need a rebase, and then I can land it.

Please comment here with a link to a rebased branch, and I'll pull it in.

Loading

@Hypnosphi
Copy link
Contributor

@Hypnosphi Hypnosphi commented Aug 10, 2021

Done, the branch is same as before

Loading

@aladdin-add
Copy link
Contributor

@aladdin-add aladdin-add commented Aug 13, 2021

friendly ping @ljharb

Loading

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Aug 13, 2021

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

Loading

@ljharb ljharb force-pushed the no-unused-modules-dynamic-imports branch 2 times, most recently from eee0cc8 to 4f338bf Aug 14, 2021
@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 Aug 14, 2021
@ljharb
Copy link
Collaborator

@ljharb 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?

Loading

@ljharb ljharb marked this pull request as draft Aug 14, 2021
@Hypnosphi
Copy link
Contributor

@Hypnosphi Hypnosphi commented Aug 30, 2021

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

Loading

@ljharb
Copy link
Collaborator

@ljharb 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.

Loading

@ljharb ljharb force-pushed the no-unused-modules-dynamic-imports branch 2 times, most recently from 63c3c5a to b724db8 Aug 30, 2021
ljharb added a commit to ljharb/eslint-plugin-import that referenced this issue Aug 30, 2021
ljharb added a commit to ljharb/eslint-plugin-import that referenced this issue Aug 30, 2021
ljharb added a commit to Hypnosphi/eslint-plugin-import that referenced this issue 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 added a commit to Hypnosphi/eslint-plugin-import that referenced this issue 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 Sep 14, 2021
ljharb
ljharb approved these changes Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants