-
Notifications
You must be signed in to change notification settings - Fork 84
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(tsconfig): respect path mapping #136
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing. I think this logic best belongs in filing-cabinet: https://github.com/dependents/node-filing-cabinet/blob/754d3889e8ba53283a126eb69f08c71b760c8c2b/index.js#L214. That's the module that deals with resolving paths based on module types. Since this logic is specific to typescript path resolution, it fits well there.
It would be awesome to provide a test with this code once it's ported to filing-cabinet. There must be something in the lerna repo that could be added to the filing-cabinet suite so that we can refactor freely without worrying about breaking the resolution logic you'll add. See other TS path resolution tests here: https://github.com/dependents/node-filing-cabinet/blob/master/test/test.js#L391.
Thanks again for the effort here, but heavy TS logic doesn't belong in this module.
return createMatchPath(absoluteBaseUrl, config.tsConfig.compilerOptions.paths) | ||
} | ||
return (alias) => undefined; | ||
})() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you create intermediate IIFEs for this and https://github.com/dependents/node-dependency-tree/pull/136/files#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R130? It doesn't seem necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrjoelkemp That is to prevent using let
over const
. (A matter of personal preference, so I understand even if you don't like it.)
For example, let's say we should initialize foo
by some condition.
// In this example, assume a ternary operator cannot be used,
// as if-else blocks can be large in real cases.
let foo
if (bar) {
foo = 1
} else {
foo = 2
}
foo
is declared as let
, so it can be mutated from somewhere else when it should not be.
To prevent this, some (especially functional) languages provides syntax sugar or special approach.
For instance, Rust can treat if-else
as an "expression", not a "statement".
// In Rust, pure `let` without `mut` keyword is equivalent to `const` in Javascript.
let foo = if bar {
1
} else {
2
};
In Javascript, calling IIFE for variable initialization is a similar pattern.
// foo now becomes `const` !
const foo = (() => {
if (bar) {
return 1
}
return 2
})()
Thanks! I created new PRs: |
Hi!
First, thanks for this awesome package!
I tested this PR against https://github.com/jjangga0214/ts-yarn-lerna-boilerplate, and got success.
I used tsconfig-path for path mapping.
Thanks.
Close #135