search

pzavolinsky / ts-unused-exports

ts-unused-exports finds unused exported symbols in your Typescript project
MIT License
749 stars 49 forks source link

check for .d.ts files for absolute paths #141

Closed davidparkagoda closed 4 years ago

mrseanryan commented 4 years ago

hi @davidparkagoda thanks for the pull request.

Can you give a description of what this PR is about?

It looks like a test to see that we don't get false positives, or errors, if importing from a d.ts file ?

davidparkagoda commented 4 years ago

Yes that is correct. When your tsconfig does not have absolute paths (baseUrl) nor paths (aliases) we do not have this problem.

I also wanted to fix the with-paths problem however it looks like I have to send a pr request to tsconfig-paths because it does it strip .d.ts correctly.

mrseanryan commented 4 years ago

hi @davidparkagoda

I'm still not sure I understand. Please bear with me - here is a summary of my understanding - perhaps you can read it and see if this is correct:

  1. There is a bug where there are false positives, if importing from a d.ts file

    • this only occurs if tsconfig uses absolute paths (baseUrl) or uses aliases (paths)
    • question: why does the test in this PR pass? does the test need to be changed, to reproduce the problem?

  2. There is another problem, involving with-paths. Can you describe this problem?

Thanks David, Sean

davidparkagoda commented 4 years ago

Yea no problem.

  2. the second bug is with-paths. You can add the extension .d.ts in this array, https://github.com/pzavolinsky/ts-unused-exports/blob/783815f950034540529a23788f0ad9da0df14537/src/parser/import.ts#L13 however tsconfig-paths does not strip .d from the file.

I created another pull request to fix it in tsconfig-paths to correctly strip extensions correctly regardless of how many . there are. https://github.com/dividab/tsconfig-paths/pull/124

mrseanryan commented 4 years ago

hi @davidparkagoda
thank you for the explanation.

This fix looks good.

To proceed, I would suggest:

like this:

### Changed

- .... (summary of the fix) ...

For the 2nd issue - I would suggest opening a separate issue/PR for that.

mrseanryan commented 4 years ago

Oh, and this branch needs to be rebased on master :)

mrseanryan commented 4 years ago

Hi @davidparkagoda your fix is useful - so to get it released, I opened a new PR (#150), that has a CHANGELOG entry.

So I think PR can be closed.

For the bug with with-paths it could be good to have a separate PR, with suitable test.

Alternatively, If you have an example that I could use to reproduce that problem, that would be great.

Thank you for your contribution!