-
Notifications
You must be signed in to change notification settings - Fork 105
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
fix: allow absolute baseUrl in tsconfig.json #174
Conversation
src/config-loader.ts
Outdated
const absoluteBaseUrl = path.join(tsConfigDir, loadResult.baseUrl); | ||
const absoluteBaseUrl = path.isAbsolute(loadResult.baseUrl) | ||
? loadResult.baseUrl | ||
: path.join(tsConfigDir, loadResult.baseUrl); |
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.
Use this instead please:
const absoluteBaseUrl = path.resolve(tsConfigDir, loadResult.baseUrl);
@nwalters512 Could you also add an entry for this PR in the CHANGELOG under unreleased heading ? With that it should be good to merge. |
Hey @nwalters512, would you mind adding this to
|
I'm on it! Thanks for getting this reviewed. |
@jonaskello LGTM |
}); | ||
|
||
const successResult = result as ConfigLoaderSuccessResult; | ||
assert.equal(successResult.absoluteBaseUrl, "/baz"); |
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.
Oops, this is using assert
which is undefined.
@jonaskello Would be nice to run test CI on PRs!
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 catch! Looks like my PR predated the move to Jest in #186. I can patch up this test if you aren't already working on that.
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'll let you handle it, thanks!
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.
@aleclarson Yes, definitely would be nice to run the tests in CI :-). It used to work but perhaps it was broken by the move to github actions. I'll see if I can get that running again.
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.
PR with passing test suite: #211
configLoader
assumed thatbaseUrl
would always be relative path, but the TypeScript compiler allowsbaseUrl
to be an absolute path as well. This PR updates theconfigLoader
to correctly computeabsoluteBaseUrl
if thebaseUrl
in the TypeScript config file is already absolute.Note that the same thing was already being done if
explicitParams
was provided.