-
Notifications
You must be signed in to change notification settings - Fork 12k
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
Stop karma gracefully, test watch mode, and check syntactic diagnostics on rebuilds #12824
Conversation
// calls `disconnectBrowsers(code);` | ||
// karmaServer.close(); | ||
// Karma only has the `stop` method start with 3.1.1, so we must defensively check. | ||
if (karmaServer.stop && typeof karmaServer.stop === 'function') { |
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.
But isn’t karma 3.1.1 are direct dependency of build-angular? So what case should karmaServer.stop doesn’t exist? Apart from if someone has yarn resolutions.
Small note this feature was added in Karma 3.1.0
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.
Karma is actually not a direct dependency of build-angular. It's a dependency of the project instead, like typescript, protractor and tslint.
@@ -9,6 +9,11 @@ import { Diagnostic, Diagnostics, Program } from '@angular/compiler-cli'; | |||
import * as ts from 'typescript'; | |||
import { time, timeEnd } from './benchmark'; | |||
|
|||
export const enum DiagnosticMode { | |||
All, |
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 const needed? Also, you could use flags so it's possible to make combinations. Unless this is meant to be used as an internal only API, which it doens't look like.
Something like:
export enum DiagnosticMode {
Syntactic = 1 << 0,
Semantic = 1 << 1,
All = Syntactic | Semantic,
Default = All,
}
What do you think?
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 think that's much better really. Updated, thank you!
@@ -52,34 +58,44 @@ export function gatherDiagnostics( | |||
} | |||
} | |||
|
|||
const gatherSyntacticDiagnostics = mode == DiagnosticMode.All || mode == DiagnosticMode.Syntactic; |
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.
With the above you can just use mode & DiagnosticMode.Syntactic != 0
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.
Updated to (mode & DiagnosticMode.Syntactic) != 0
because !=
seems to have precedence over &
.
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.
JavaScript!
Hi @filipesilva! This PR has merge conflicts due to recent upstream merges. |
770aa23
to
e35fd39
Compare
e35fd39
to
e504c19
Compare
Syntactic errors were only being reported in the type checker when it was running. This caused rebuilds to finish successfully if they had syntactic errors, which could lead to very weird behaviour on rebuilds.
e504c19
to
1453b9d
Compare
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Followup to karma-runner/karma#3153