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

check for .d.ts files for path aliases #151

Conversation

davidparkagoda
Copy link
Contributor

@davidparkagoda davidparkagoda commented Jul 6, 2020

Summary:

Tries to fix a bug where there are false positives, if importing from a d.ts file.
This only occurs if tsconfig uses aliases (paths).

This PR would need another PR to be merged in
dividab/tsconfig-paths#124

Right now tsconfig-paths is returning file paths with the extension stripped, src/something/hi.ts => src/something/hi.
However this extension stripping only removes 1 level of . so when we matched .d.ts files it will incorrectly strip the extension to be like src/something/hi.d.ts => src/something/hi.d.

@mrseanryan
Copy link
Collaborator

hi @davidparkagoda

question -
as an alternative to waiting for the ts-config-paths PR,
could we 'patch' our code to get around this:

will incorrectly strip the extension to be like src/something/hi.d.ts => src/something/hi.d.

We could add a TODO to remove our 'patch' once ts-config-paths is updated.

@davidparkagoda
Copy link
Contributor Author

Yes we could definitely do that but it does not feel great.

Even if we do this 'patch', the workarounds that I see are not great.

We could do something like
matchedPath.replace(/\.d$/, '') but that seems a bit too broad.
or
after we get the matchedPath we check if the file exists matchPath + '.ts' but that is an extra file check.

If you are okay with one of those solutions or have a better workaround I am willing to make the change

@mrseanryan
Copy link
Collaborator

| We could do something like
| matchedPath.replace(/.d$/, '') but that seems a bit too broad.

I think this would work?

Or - in which cases would this cause a problem ?

@davidparkagoda
Copy link
Contributor Author

davidparkagoda commented Jul 9, 2020

I know this is kind of edge case but a files named like

file.d.tsx
file.d.js
file.d.jsx

you would need import from javascript like
import { ExportedThing } from 'file.d'; (for file.d.js, ...)

but ts-unsused-exports would be looking for file rather than file.d if I make the matchedPath.replace(/.d$/' '') change

@mrseanryan
Copy link
Collaborator

Hmm wouldn't that also happen with the fix for tsconfig-paths?

@davidparkagoda
Copy link
Contributor Author

No.

When tsconfig-paths is iterating the list of extensions that we pass to it, I modified it so that it will note the extension length of the matched path when found. This is make it so that we only strip out the length of the of the matched extension.

You will not be able to have the same kind of "bug" with the fix for tsconfig-paths without having a really weird webpack or configuration

const EXTENSIONS = ['.d.ts', '.ts', '.tsx', '.d.js', '.js', '.jsx'];
Now if you imported file, which through weird configuration, points to file.d.js, tsconfig-paths will return file which is correct given the extensions that we passed to tsconfig-paths

However if you imported file.d, tsconfig-paths will try
'file.d' + '.d.js', which does not exist
'file.d' + .js'which does exist.tsconfig-pathsalso knows the matched extension so it only strips.js`

@mrseanryan
Copy link
Collaborator

mrseanryan commented Jul 10, 2020

I see. So in such cases, tsconfig-paths checks for file exists.

Question is that covered by this approach:

after we get the matchedPath we check if the file exists matchPath + '.ts' but that is an extra file check.

@davidparkagoda
Copy link
Contributor Author

yes this would work. something like

if (matchedPath.endsWith('.d')) {
    if (matchedPath + '.ts') file exists {
        matchedPath = matchedPath.slice(0, -2);
    }
}

@mrseanryan
Copy link
Collaborator

mrseanryan commented Aug 25, 2020

@davidparkagoda

is this PR completed ?

(it looks good to me).

Thanks,
Sean

@davidparkagoda
Copy link
Contributor Author

davidparkagoda commented Aug 26, 2020

@mrseanryan yup everything should be good. Please go ahead and do anything to get this merged in :D

@mrseanryan
Copy link
Collaborator

@davidparkagoda
Merged it to master (used a separate PR due to some github/travis glitch).
Hopefully it will be released soon.

Thank you for your contribution !

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

Successfully merging this pull request may close these issues.

2 participants