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

Switches the bundlePackageName error message to be a suggestion, and not a compiler error #41510

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

@orta
Copy link
Member

@orta orta commented Nov 12, 2020

Alternative to #41499 where the compiler flag instead recommends using bundlePackageName on the out in your JSON instead of outright removing the feature.

@orta orta changed the base branch from master to release-4.1 Nov 12, 2020
@orta orta changed the base branch from release-4.1 to master Nov 12, 2020
Copy link
Member

@weswigham weswigham left a comment

I think this is a good, low-risk compromise, since this doesn't change much from what we shipped in the beta, keeps the fundamental feature in place, and still provides a suggestion in the IDE so people can know something's up. 👍

Loading

"The `bundledPackageName` option must be provided when using outFile and node module resolution with declaration emit.": {
"category": "Error",
"The `bundledPackageName` option should be provided when using outFile and node module resolution with declaration emit.": {
"category": "Suggestion",
Copy link
Member

@sheetalkamat sheetalkamat Nov 12, 2020

Choose a reason for hiding this comment

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

this is currently not desirable i think... because we do not distinguish between suggestion or error and build will fail with corresponding exit code instead.

Loading

Copy link
Member

@weswigham weswigham Nov 12, 2020

Choose a reason for hiding this comment

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

Yeah that's fair, we do kinda have a separate suggestionDiagnostics list in the checker.
I don't think we have one in program yet, but we'd need to add one and concatenate it with the checker's suggestions. (Inside getSuggestionDiagnostics in program.ts)

Loading

Copy link
Member

@sheetalkamat sheetalkamat Nov 12, 2020

Choose a reason for hiding this comment

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

but that means it would reported only in editor ... and not in build i am not sure thats desirable either... program construction messages should be consistent in my opinion..

Loading

Copy link
Member

@weswigham weswigham Nov 12, 2020

Choose a reason for hiding this comment

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

I mean, suggestions are only actionable in the editor at present, no? We don't have a --showSuggestions compiler flag or anything to print them on the command line at present?

Loading

Copy link
Member

@sheetalkamat sheetalkamat Nov 12, 2020

Choose a reason for hiding this comment

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

currently suggestions are errors under noUnusedLocals like flags as far as i know so there is way to get those.

Loading

Copy link
Member

@weswigham weswigham Nov 13, 2020

Choose a reason for hiding this comment

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

Diagnostics.await_has_no_effect_on_the_type_of_this_expression is unconditionally a suggestion (see its usage in checkAwaitExpression), as is Diagnostics.Numeric_literals_with_absolute_values_equal_to_2_53_or_greater_are_too_large_to_be_represented_accurately_as_integers in checkNumericLiteralValueSize, so we do already have suggestions you can never see on the command line by conversion into an error.

Loading

Copy link
Member

@weswigham weswigham Nov 13, 2020

Choose a reason for hiding this comment

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

(And these still have value because we use them to trigger quickfixes!)

Loading

@sandersn sandersn added this to Not started in PR Backlog Nov 23, 2020
@sandersn sandersn moved this from Not started to Needs review in PR Backlog Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
PR Backlog
  
Waiting on reviewers
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants