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

feat(tsLookup): path mapping #100

Merged
merged 4 commits into from
Apr 30, 2022

Conversation

jjangga0214
Copy link
Contributor

@jjangga0214 jjangga0214 commented Aug 16, 2021

Hi!

ts.resolveModuleName does not handle Path Mapping.

const namedModule = ts.resolveModuleName(dependency, filename, compilerOptions, host);

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:

jjangga0214 added a commit to jjangga0214/node-dependency-tree that referenced this pull request Aug 16, 2021
A PR pending in filingc-cabinet should be merged and published before
this to be merged. REF: dependents/node-filing-cabinet#100
@pauldijou
Copy link

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
Copy link
Contributor Author

jjangga0214 commented Aug 20, 2021

@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
Copy link

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
Copy link
Contributor Author

jjangga0214 commented Aug 20, 2021

@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

index.js Outdated Show resolved Hide resolved
@mrjoelkemp
Copy link
Collaborator

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
Copy link
Contributor Author

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

@nickhodaly
Copy link

@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
Copy link
Contributor Author

jjangga0214 commented Sep 23, 2021

@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
Copy link
Contributor Author

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

@jjangga0214
Copy link
Contributor Author

jjangga0214 commented Jan 26, 2022

@mrjoelkemp Ready to merge IMHO.

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

@jjangga0214
Copy link
Contributor Author

@mrjoelkemp May I ping?

@mrjoelkemp mrjoelkemp merged commit 05b02b9 into dependents:master Apr 30, 2022
@mrjoelkemp
Copy link
Collaborator

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

@jjangga0214 jjangga0214 deleted the feat/tsLookup/path-mapping branch May 29, 2022 02:55
mrjoelkemp pushed a commit to jjangga0214/node-dependency-tree that referenced this pull request Aug 3, 2022
A PR pending in filingc-cabinet should be merged and published before
this to be merged. REF: dependents/node-filing-cabinet#100
jjangga0214 added a commit to jjangga0214/node-dependency-tree that referenced this pull request May 27, 2023
A PR pending in filingc-cabinet should be merged and published before
this to be merged. REF: dependents/node-filing-cabinet#100
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