-
Notifications
You must be signed in to change notification settings - Fork 44
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
feat(tsLookup): path mapping #100
feat(tsLookup): path mapping #100
Conversation
A PR pending in filingc-cabinet should be merged and published before this to be merged. REF: dependents/node-filing-cabinet#100
Hi @jjangga0214 , thanks for doing that, I was facing the same issue on my project. Quick question, I tried to applied that code but it seemed to fail for me because, when calling Am I missing something? Like another way to setup those extensions properly. |
For example, it works like this. const { createMatchPath } = require('tsconfig-paths')
const compilerOptionsPaths = {
"#foo/*": ["foo/src/*"],
"#bar/*": ["bar/src/*"],
}
const absoluteBaseUrl = '/path/to/packages/'
// REF: https://github.com/dividab/tsconfig-paths#creatematchpath
const tsMatchPath = createMatchPath(absoluteBaseUrl, compilerOptionsPaths)
const resolvedTsAliasPath = tsMatchPath("#foo/hello/qux")
// All extensions are reserved.
console.log(tsMatchPath("#foo/hello/qux")) // '/path/to/packages/foo/src/hello/qux'
console.log(tsMatchPath("#foo/hello/qux.js")) // '/path/to/packages/foo/src/hello/qux.js'
console.log(tsMatchPath("#foo/hello/qux.jsx")) // '/path/to/packages/foo/src/hello/qux.jsx'
console.log(tsMatchPath("#foo/hello/qux.ts")) // '/path/to/packages/foo/src/hello/qux.ts'
console.log(tsMatchPath("#foo/hello/qux.tsx")) // '/path/to/packages/foo/src/hello/qux.tsx' Please note that export interface MatchPath {
(requestedModule: string, readJson?: Filesystem.ReadJsonSync, fileExists?: (name: string) => boolean, extensions?: ReadonlyArray<string>): string | undefined;
} I guess this behavior might be the reason for your issue. If you don't actually have files, but want to test virtually, you can do so by, console.log(tsMatchPath("#foo/hello/qux", undefined, () => true)) // '/path/to/packages/foo/src/hello/qux'
console.log(tsMatchPath("#foo/hello/qux.js", undefined, () => true)) // '/path/to/packages/foo/src/hello/qux.js'
console.log(tsMatchPath("#foo/hello/qux.jsx", undefined, () => true)) // '/path/to/packages/foo/src/hello/qux.jsx'
console.log(tsMatchPath("#foo/hello/qux.ts", undefined, () => true)) // '/path/to/packages/foo/src/hello/qux.ts'
console.log(tsMatchPath("#foo/hello/qux.tsx", undefined, () => true)) // '/path/to/packages/foo/src/hello/qux.tsx' If the output is still different from expectation, would you please provide a simple reproduction repo? Thanks :) |
Sure, here is the repro for me, considering a project with a
// index.js
const { createMatchPath } = require('tsconfig-paths');
const compilerOptionsPaths = {
"~/*": ["./src/*"],
}
const absoluteBaseUrl = __dirname;
const tsMatchPath = createMatchPath(absoluteBaseUrl, compilerOptionsPaths)
const resolvedTsAliasPath = tsMatchPath("~/test")
const resolvedTsAliasPath2 = tsMatchPath("~/test", undefined, undefined, ['.ts'])
console.log(resolvedTsAliasPath) // undefined
console.log(resolvedTsAliasPath2) // [root folder]/tsPathsTest/src/test |
Oh, thanks! I see what you're saying. I used
export function matchFromAbsolutePaths(
absolutePathMappings: ReadonlyArray<MappingEntry.MappingEntry>,
requestedModule: string,
readJson: Filesystem.ReadJsonSync = Filesystem.readJsonFromDiskSync,
fileExists: Filesystem.FileExistsSync = Filesystem.fileExistsSync,
extensions: Array<string> = Object.keys(require.extensions), // <= This one!
mainFields: string[] = ["main"]
): string | undefined { So, yes, you're right. I have to patch this PR from Edit: Patched in a6740c2 |
Looks good so far. Ping for a review when you have some tests to make sure we don't break this functionality in the future. Thanks for the effort! |
@mrjoelkemp I've been busy these days. I may add tests in the next week or so. Thanks! |
@jjangga0214 Any plans on merging this soon? I am running into a similar situation. |
@nickhodaly I've been busier than initially expected, seriously, haha. The code is actually completed(unless a new issue is found), and the only job left is to add test cases. If you have time, you're good to create a PR! (probably against my repo jjangga0214/node-filing-cabinet:feat/tsLookup/path-mapping ) Otherwise, I will do that on next week. |
Hmm, I've been seriously busy. I will do that this month. |
@mrjoelkemp Ready to merge IMHO. @nickhodaly @pauldijou I really wanted(and should) to complete this sooner. Sincerely thank you for patience/contribution. |
@mrjoelkemp May I ping? |
7e9ef2b
to
bc9913f
Compare
@jjangga0214 really sorry about the delay here, especially because you put so much time into this. I resolved merge conflicts and included this in a new minor release. Excellent work here. Thanks for contributing. |
A PR pending in filingc-cabinet should be merged and published before this to be merged. REF: dependents/node-filing-cabinet#100
A PR pending in filingc-cabinet should be merged and published before this to be merged. REF: dependents/node-filing-cabinet#100
Hi!
ts.resolveModuleName
does not handle Path Mapping.node-filing-cabinet/index.js
Line 227 in 754d388
Therefore, currently, Path Mapping is just ignored by
filing-cabinet
.Fortunately, there is a library tsconfig-path, thus I used it.
Path Mapping cannot be resolved without knowing an absolute path to tsconfig file (Accurately speaking, an absolute path to
baseUrl
, which is calculated from an absolute path to tsconfig file). (And this is nottsconfig-path
's specific requirement. This is required by the concept of Path Mapping itself.)Thus I added a new option
options.tsConfigPath
. As I wrote on readme, the option is only needed whenoptions.tsConfig
is given as an object AND Path Mapping is needed. Ifoptions.tsConfig
is given as a string,options.tsConfigPath
is calculated from it when it's not already given.This is not a breaking change, existing code would work as it used to do.
I personally tested this PR, but did not write new test cases, by the way.
Thanks.
Related to: