-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
add tsconfig.json compilerOptions.paths support #144
Conversation
…ild into feature/tsconfig-paths
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 taking this feature on! This is exciting. I left some comments inline.
Also, please remove the npm/esbuild-linux-arm64/bin/esbuild
binary from this PR.
internal/resolver/resolver.go
Outdated
if dirInfo.tsConfigJson.paths != nil { | ||
for key, originalPaths := range dirInfo.tsConfigJson.paths { | ||
for _, originalPath := range originalPaths { | ||
if matched, err := regexp.MatchString("^"+key, path); matched && err == nil { |
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.
Using a regex to match this isn't correct. The pattern is an asterisk, which causes a regex to repeat the previous character zero or more times. That would mean a pattern foo*
would also match a directory named foooo
and fo
. I took some time to look at what the TypeScript compiler itself does. The relevant code is in tryLoadModuleUsingPaths()
here.
Other relevant code is tryParsePattern()
here:
export function tryParsePattern(pattern: string): Pattern | undefined {
// This should be verified outside of here and a proper error thrown.
Debug.assert(hasZeroOrOneAsteriskCharacter(pattern));
const indexOfStar = pattern.indexOf("*");
return indexOfStar === -1 ? undefined : {
prefix: pattern.substr(0, indexOfStar),
suffix: pattern.substr(indexOfStar + 1)
};
}
and isPatternMatch()
here:
function isPatternMatch({ prefix, suffix }: Pattern, candidate: string) {
return candidate.length >= prefix.length + suffix.length &&
startsWith(candidate, prefix) &&
endsWith(candidate, suffix);
}
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.
thank you! i've added some commits to handle this more like ts compiler, but i haven't written the test case where *
is at the start or middle of the path.
internal/resolver/resolver.go
Outdated
|
||
} | ||
} else { | ||
basePath := r.fs.Join(*dirInfo.tsConfigJson.absPathBaseUrl, path) |
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.
It looks like the TypeScript compiler still respects baseUrl
if no paths
matched, so we should do that too.
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! resolved on 2587e08
internal/resolver/resolver.go
Outdated
@@ -578,13 +601,43 @@ func (r *resolver) loadAsFileOrDirectory(path string) (string, bool) { | |||
return "", false | |||
} | |||
|
|||
func resolvePathWithoutStar(from, path string) (string, error) { | |||
replaced := strings.Replace(path, "/*", "", -1) | |||
return fp.Join(from, replaced), nil |
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.
The code in this file is careful to always use the fs
interface in the resolver
instead of path/filepath
. All platform-dependent things are supposed to happen inside the fs
interface so that the resolver can be platform-independent. That interface uses the real path/filepath
module for the esbuild command and the path
module for tests. That way tests don't do different things on Windows.
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.
thank you! resolved in 444bfae
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 getting this work started. I can take it from here.
files: map[string]string{ | ||
"/Users/user/project/src/entry.js": ` | ||
import fn from 'core/test' | ||
import fn2 from 'testing/test' |
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.
It appears this case isn't actually supposed to work. At least the TypeScript compiler throws an error here. I can do the testing work and make adjustments to the algorithm as appropriate.
if dirInfo.tsConfigJson.absPathBaseUrl != nil { | ||
if dirInfo.tsConfigJson.paths != nil { | ||
for key, originalPaths := range dirInfo.tsConfigJson.paths { | ||
for _, originalPath := range originalPaths { |
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.
This doesn't follow the behavior of the TypeScript compiler which prefers exact matches over pattern matches and prefers matches with longer prefixes over matches with shorter prefixes. That's ok though, I'll fix it.
No description provided.