Skip to content
This repository was archived by the owner on Jun 16, 2025. It is now read-only.

Conversation

rexxars
Copy link
Contributor

@rexxars rexxars commented Sep 5, 2021

Fixes #568
Fixes #526

I'm not sure if this is the best approach, given none of the other detectors seems to be using the second deps argument, but the alternative would be to change the returned value from an array of strings to an object that includes dependency names and an additional property for the type of import, or similar.

rexxars added a commit to sanity-io/sanity that referenced this pull request Sep 6, 2021
This is only required to make depcheck pass - we're only importing the types,
which are given by `@types/inquirer`, but depcheck doesn't understand this.

I created a PR[1] against depcheck to fix this, but it is hard to know when/if
this patch will land, so this seems like a reasonable workaround given it is
only a development dependency.

[1]: depcheck/depcheck#670
rexxars added a commit to sanity-io/sanity that referenced this pull request Sep 6, 2021
This is only required to make depcheck pass - we're only importing the types,
which are given by `@types/inquirer`, but depcheck doesn't understand this.

I created a PR[1] against depcheck to fix this, but it is hard to know when/if
this patch will land, so this seems like a reasonable workaround given it is
only a development dependency.

[1]: depcheck/depcheck#670
rexxars added a commit to sanity-io/sanity that referenced this pull request Sep 6, 2021
This is only required to make depcheck pass - we're only importing the types,
which are given by `@types/inquirer`, but depcheck doesn't understand this.

I created a PR[1] against depcheck to fix this, but it is hard to know when/if
this patch will land, so this seems like a reasonable workaround given it is
only a development dependency.

[1]: depcheck/depcheck#670
rexxars added a commit to sanity-io/sanity that referenced this pull request Sep 6, 2021
This is only required to make depcheck pass - we're only importing the types,
which are given by `@types/inquirer`, but depcheck doesn't understand this.

I created a PR[1] against depcheck to fix this, but it is hard to know when/if
this patch will land, so this seems like a reasonable workaround given it is
only a development dependency.

[1]: depcheck/depcheck#670
Copy link
Member

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

Seems like an easy enough fix 👍

ricokahler pushed a commit to sanity-io/sanity that referenced this pull request Sep 7, 2021
This is only required to make depcheck pass - we're only importing the types,
which are given by `@types/inquirer`, but depcheck doesn't understand this.

I created a PR[1] against depcheck to fix this, but it is hard to know when/if
this patch will land, so this seems like a reasonable workaround given it is
only a development dependency.

[1]: depcheck/depcheck#670
rexxars added a commit to sanity-io/sanity that referenced this pull request Sep 7, 2021
This is only required to make depcheck pass - we're only importing the types,
which are given by `@types/inquirer`, but depcheck doesn't understand this.

I created a PR[1] against depcheck to fix this, but it is hard to know when/if
this patch will land, so this seems like a reasonable workaround given it is
only a development dependency.

[1]: depcheck/depcheck#670
rexxars added a commit to sanity-io/sanity that referenced this pull request Sep 7, 2021
This is only required to make depcheck pass - we're only importing the types,
which are given by `@types/inquirer`, but depcheck doesn't understand this.

I created a PR[1] against depcheck to fix this, but it is hard to know when/if
this patch will land, so this seems like a reasonable workaround given it is
only a development dependency.

[1]: depcheck/depcheck#670
rexxars added a commit to sanity-io/sanity that referenced this pull request Sep 7, 2021
This is only required to make depcheck pass - we're only importing the types,
which are given by `@types/inquirer`, but depcheck doesn't understand this.

I created a PR[1] against depcheck to fix this, but it is hard to know when/if
this patch will land, so this seems like a reasonable workaround given it is
only a development dependency.

[1]: depcheck/depcheck#670
Copy link
Member

@rumpl rumpl left a comment

Choose a reason for hiding this comment

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

Code looks good to me, sorry for the late review...

@rumpl
Copy link
Member

rumpl commented Nov 10, 2021

@rexxars thanks for the PR, could you amend your commit and force push to kickstart the CI please, I have no idea why I can run the actions on your PR...

@rexxars rexxars force-pushed the fix/ts-types-import branch from 3ae07d3 to 8643cd3 Compare November 10, 2021 16:51
@rexxars
Copy link
Contributor Author

rexxars commented Nov 10, 2021

Sure. Rebased.

@codecov
Copy link

codecov bot commented Nov 10, 2021

Codecov Report

Merging #670 (8643cd3) into main (73c2bec) will decrease coverage by 0.19%.
The diff coverage is 84.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #670      +/-   ##
==========================================
- Coverage   98.64%   98.45%   -0.18%     
==========================================
  Files          49       49              
  Lines        1023     1031       +8     
==========================================
+ Hits         1009     1015       +6     
- Misses         14       16       +2     
Impacted Files Coverage Δ
src/utils/configuration-reader.js 100.00% <ø> (ø)
src/cli.js 95.56% <75.00%> (-4.44%) ⬇️
src/detector/importDeclaration.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a687b10...8643cd3. Read the comment docs.

@rumpl rumpl merged commit 180e05a into depcheck:main Nov 10, 2021
@rumpl
Copy link
Member

rumpl commented Nov 10, 2021

Thanks @rexxars

@rexxars rexxars deleted the fix/ts-types-import branch November 10, 2021 17:55
@rexxars
Copy link
Contributor Author

rexxars commented Nov 26, 2021

Thanks for merging! Any chance of seeing this one in a release anytime soon?

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

Successfully merging this pull request may close these issues.

False positive: types-only dependencies Typescript dependencies with format @types/xxxx are incorrectly reported when using type imports
3 participants