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

fix: tolerate an undefined baseUrl compiler option #208

Merged
merged 4 commits into from May 2, 2022

Conversation

aleclarson
Copy link
Contributor

@aleclarson aleclarson commented Apr 20, 2022

Fixes #143

This is a breaking change.

@@ -22,7 +22,7 @@ export interface ConfigLoaderParams {
export interface ConfigLoaderSuccessResult {
resultType: "success";
configFileAbsolutePath: string;
baseUrl: string;
baseUrl?: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why it's a breaking change

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making a previously required property optional is not a breaking change IMO, especially in this case:

  • It won't change anything for projects having a baseUrl
  • There's virtually no projects using tsconfig-paths that currently do not have a baseUrl, because it was basically broken

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR introduces a compiler error into any downstream package that happens to use loadConfig and expects the baseUrl property to only ever be a string. If a change can cause downstream compiler errors, I consider it breaking.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I was not aware of the public API, my bad!

@jonaskello
Copy link
Member

@aleclarson Could you also add an entry for this PR in the CHANGELOG under unreleased heading, noting it as a breaking change.

@aleclarson
Copy link
Contributor Author

LGTM

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.

Allow missing baseUrl for TS 4.1+
3 participants