Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 path mapping support to ESM and CJS loaders #1585
base: main
Are you sure you want to change the base?
Add path mapping support to ESM and CJS loaders #1585
Changes from 8 commits
3be1fde
c542d98
0cbfa6c
e7082b1
69a397b
2fcb982
48f5262
a19454b
32e26e8
362935b
e573fd7
0012b22
b6352e3
85643fd
9c46688
bede1b6
80746c2
885b7b1
c3dbe73
8c5fd23
e66d236
23f40b1
54fdbba
78652b2
b8e6fb7
2973399
5ffd905
d200301
c621af0
ec038c1
d471d1d
d5728dc
5b686e1
cb3706e
d00bf73
398db86
a91d5d7
7829b54
9fd944e
4e0d363
09cc514
70d9bf5
e50abd4
73d6acc
d02cbed
283309d
f2e2f65
93ac2ac
bf7bfcd
c26b9dd
09ebf63
e0af738
31acd50
36377fc
56ef378
4dcff42
ba176bf
38ad3f5
1687de9
8e9b117
806ce74
0fd8a5c
02818e3
6faedd9
64bb679
058d2e2
4a3b6b4
dc7e950
c5ea9b0
7bfe31a
c8c35ca
99ca516
4fefc57
3a0067e
78e3eda
4f5bc35
ef926b9
7552fc4
6632481
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
compilerOptions.baseUrl
guaranteed to be an absolute path, or do we need to resolve it relative to the tsconfig's location?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.
Good point. I assumed that we always get the configuration from
ts.readConfigFile()
insrc/configuration.ts
and the function resolves the base URL relative to the config file. But there are probably other ways to set the base URL like through the command line, API, or environment.To address this I think it make sense to resolve any base URL that is not a URL but a relative path to the current directory.
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.
I see, we almost always get the configuration from
ts.readConfigFile()
, so we should be good. I think the only way to set it otherwise is via the API? Since we don't support all oftsc
's command-line flags.Maybe the correct approach is to resolve
baseUrl
once withincreate()
so that we have a reliable, absolutebaseUrl
to be used elsewhere.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.
Alternatively, we could require
baseUrl
to be absolute and throw an error otherwise. This would allow us to avoid implicit, environment-dependent behavior for users of the API. This might prevent some confusion when path mapping doesn’t work as expected but only fails at the resolution stage instead of the construction stage. (For example a user may think the base URL they set on the API is considered relative totsconfig.json
and their scripts work whenever they are run from the project root. But then they run the script from a subdirectory and it fails.)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.
Sounds good, let's do that. In
create()
we throw an error if baseUrl is not absolute.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.
Actually, scratch that, seems we always pass options through TypeScript's API to be normalized, even when provided via our API:
So I guess we're all good.