Skip to content

feat: add noTypeDefinitions option #130

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

Merged
merged 3 commits into from
Apr 17, 2021
Merged

Conversation

strangedev
Copy link
Contributor

Hi there 👋

first of all: Thank you for this useful project! 😄

I use a lot of TypeScript and I've found a small inconvenience when using your project with it. Filing cabinet has two ways of resolving TS imports, the first one - the one that is used by default - resolves these to the type definitions. The other one resolves to the JavaScript implementations instead.

So why should you be able to resolve to the .js files and not to the d.ts files?
The problem with just resolving to the type definitions is, that definition files don't usually contain imports of all dependencies, so we're missing transitive dependencies here.

Picture this scenario: You're using some dependency A which is written in JS, but provides TS definitions. A uses B as its dependency. When we resolve A to its definition file, that file doesn't include the import for B, so we're missing B in the dependency tree. If we resolve to the JS file instead, this doesn't happen.

So this is what I implemented here. I've add the noDefinitionFiles option from filing cabinet to this packages' options. With this option, it is possible to toggle between both behaviors. To keep the code compatible with older versions, the default is the same as before - resolve to definition files.

I've added test cases for this as well. 🎉

Also, since I want to use this with TypeScript, I've added type definitions to your package. They should be compatible with the ones provided by DefinitelyTyped and reflect the new changes, but feel free to revert this commit if you don't want to add types at this point. All changes related to the addition of types are contained in 149151f, so you're able to discard these more easily. I'd also be happy to create a second PR for the type definitions, if that's how you'd like to handle it.

@strangedev
Copy link
Contributor Author

I've just realized that there was a previous PR for this 🤔
This is the PR: de87425

Also, I forgot to adjust the README. I will fix this quickly 😄

@mrjoelkemp mrjoelkemp merged commit 6b1837f into dependents:master Apr 17, 2021
@mrjoelkemp
Copy link
Collaborator

This is great! Thanks @strangedev (and @boneskull for a similar effort). I don't use TS myself, so I appreciate the help pushing the project forward in that space. Just pushed a minor version with this change. Thanks again for contributing.

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