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

Fix problem with tslint-loader (issue #15) #62

Closed
wants to merge 1 commit into from

Conversation

stenin-nikita
Copy link

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.914% when pulling 53541df on stenin-nikita:issue/15 into 3c5eafa on dividab:master.

@jonaskello
Copy link
Member

@stenin-nikita Could you provide some reasoning behind this PR? Seems like it removes the handling of core modules in favour of a try/catch. Could you explain why this would solve the problem in #15?

@stenin-nikita
Copy link
Author

Module._resolveFilename loads core modules and modules from node_modules by default. If the module is not found, we try to resolve the module from paths in tsconfig.

node_modules/
  tslint/
  tslint-loader/ // const tslint = require('tslint')
tslint.json

The current implementation will load tslint.json from root dir. My implementation will first try load tslint package from node_modules. Therefore, tslint-loader will work correctly. This order does not break the standard module loading behavior.

@jonaskello
Copy link
Member

Ok, I think I understand the problem, I made a comment in the issue as it seems better to discuss the root cause there.

I would prefer a solution that does not involve try/catch for each resolve. In part because my view is that exceptions are for unexpected errors, but mostly because I think it will affect performance. I think maybe we can understand what is causing this and then check it before resolving instead. However to do this I would need to recreate the issue in #15.

@aleclarson
Copy link
Contributor

@jonaskello This can probably be closed, since tslint is now deprecated?

@jonaskello
Copy link
Member

Yes, let's close this one. I think most projects are using eslint now.

@jonaskello jonaskello closed this Apr 15, 2022
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.

None yet

4 participants