dependents / node-filing-cabinet

Get the file location associated with a dependency/partial's path
MIT License
79 stars 44 forks source link

feat(tsLookup): path mapping #100

Closed jjangga0214 closed 2 years ago

jjangga0214 commented 3 years ago

Hi!

ts.resolveModuleName does not handle Path Mapping.

https://github.com/dependents/node-filing-cabinet/blob/754d3889e8ba53283a126eb69f08c71b760c8c2b/index.js#L227

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 not tsconfig-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 when options.tsConfig is given as an object AND Path Mapping is needed. If options.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:

pauldijou commented 3 years ago

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 const resolvedTsAliasPath = tsMatchPath(dependency), it would only check on JavaScript extensions (like .js or .node) but not TypeScript ones. I needed to change that line to const resolvedTsAliasPath = tsMatchPath(dependency, undefined, undefined, ['.tsx', '.ts', '.js', '.jsx']) in order to make it work.

Am I missing something? Like another way to setup those extensions properly.

jjangga0214 commented 3 years ago

@pauldijou

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 tsMatchPath works only when the file really exists. Its type has an optional argument fileExists, and by default, it would check whether a file exists.

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 :)

pauldijou commented 3 years ago

Sure, here is the repro for me, considering a project with a src/test.ts in it. Are you using ts-node maybe? I'm using node directly myself.

tsPathsTest
├── index.js
└── src
    └── test.ts
// 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
jjangga0214 commented 3 years ago

@pauldijou

Oh, thanks!

I see what you're saying.

I used ts-node, and that's why I had a different result from yours.

tsconfig-paths checks extensions by require.extensions as a default, and ts-node adds additional ones (e.g. ts) to it.

https://github.com/dividab/tsconfig-paths/blob/80bc8106ee580dea5d379e462fdd4cbeb43ecfcf/src/match-path-sync.ts#L64-L71

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 tsMatchPath(dependency) to tsMatchPath(dependency, undefined, undefined, ['.js', '.jsx', '.ts', '.tsx', '.d.ts', '.node', '.json']).

Edit: Patched in a6740c2

mrjoelkemp commented 3 years ago

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!

jjangga0214 commented 3 years ago

@mrjoelkemp I've been busy these days. I may add tests in the next week or so. Thanks!

nickhodaly commented 3 years ago

@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.

jjangga0214 commented 3 years ago

@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.

jjangga0214 commented 3 years ago

Hmm, I've been seriously busy. I will do that this month. Just letting you know I still have an intention.

jjangga0214 commented 2 years ago

@mrjoelkemp Ready to merge IMHO.

@nickhodaly @pauldijou I really wanted(and should) to complete this sooner. Sincerely thank you for patience/contribution.

jjangga0214 commented 2 years ago

@mrjoelkemp May I ping?

mrjoelkemp commented 2 years ago

@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.