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: add tsconfig alias to tsConfig option #1565

Merged
merged 2 commits into from
Apr 28, 2020
Merged

Conversation

jednano
Copy link
Contributor

@jednano jednano commented Apr 24, 2020

Summary

Fixes #1564

Test plan

Added tsconfig scenario to existing relevant test.

Does this PR introduce a breaking change?

  • Yes
  • No

@coveralls
Copy link

coveralls commented Apr 24, 2020

Pull Request Test Coverage Report for Build 4561

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 92.91%

Totals Coverage Status
Change from base Build 4560: 0.03%
Covered Lines: 1120
Relevant Lines: 1155

💛 - Coveralls

@ahnpnl
Copy link
Collaborator

ahnpnl commented Apr 25, 2020

would you please also update docs ? Here is the doc entry and this is the tsConfig page

src/types.ts Show resolved Hide resolved
* - `false`: do NOT use default config file
* - `path/to/tsconfig.json`: path to a specific tsconfig file (<rootDir> can be used)
* - `{...}`: an object with inline compiler options
* - `false`: do NOT use default config file
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These bullets were not showing up correctly in Intellisense. This fixes that.

Screen Shot 2020-04-27 at 4 34 45 PM

src/types.ts Outdated
*/
packageJson?: boolean | string | object
packageJson?: false | string | object
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replacing boolean type with false reminds the user that the default value is true; thus, it would be redundant to supply true in this case.

Copy link
Owner

Choose a reason for hiding this comment

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

This would be misleading. A type of false implies that it will always be false and not that the default value will be true

Copy link
Contributor Author

@jednano jednano Apr 28, 2020

Choose a reason for hiding this comment

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

  1. An optional property with a union type of false | string | object does not at all imply that it will always be false.
  2. The default value is not true, but undefined, according to this line.
  3. This is an option. The default value is undefined. You may override that value with false | string | object. A value of true would be superfluous and redundant; thus, misleading.
  4. The proper way to indicate default values is with the @default TSDoc tag.

Copy link
Owner

Choose a reason for hiding this comment

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

My comment above was only in the context of the false part of the sum type defined. The other two (string/object) are not relevant to this discussion.

Even if a value of true would be redundant, the fact that it can be set means that the type should be boolean and not false. Even though it's redundant, adding that makes it clear in the user's setup what is happening. If it's true by virtue of being absent, it's not always clear - the user will have to know or will have to read the documentation to understand the behavior.

I agree with 4 and that's also part of the reason I'm in favor of leaving the type as boolean instead of false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not interested in fighting this, so I restored the Booleans.

@jednano jednano force-pushed the tsconfig branch 4 times, most recently from d7309a9 to 8e5091c Compare April 28, 2020 07:20
docs/user/config/tsConfig.md Show resolved Hide resolved
docs/user/config/tsConfig.md Show resolved Hide resolved
docs/user/config/tsConfig.md Show resolved Hide resolved
docs/user/config/tsConfig.md Show resolved Hide resolved
Copy link
Owner

@kulshekhar kulshekhar left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @jedmao

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.

Accept tsconfig alias to tsConfig
4 participants