From dbc2364d1689f757d7fcf7067f5824d989018a33 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Thu, 25 Jun 2020 16:15:14 +0200 Subject: [PATCH 01/20] fix(dev-infra): support running scripts from within a detached head (#37737) Scripts provided in the `ng-dev` command might use local `git` commands. For such scripts, we keep track of the branch that has been checked out before the command has been invoked. We do this so that we can later (upon command completion) restore back to the original branch. We do not want to leave the Git repository in a dirty state. It looks like this logic currently only deals with branches but does not work properly when a command is invoked from a detached head. We can make it work by just checking out the previous revision (if no branch is checked out). PR Close #37737 --- dev-infra/pr/discover-new-conflicts/index.ts | 14 +++++++------- dev-infra/pr/merge/strategies/autosquash-merge.ts | 8 ++++---- dev-infra/pr/merge/task.ts | 14 +++++++------- dev-infra/pr/rebase/index.ts | 10 +++++----- dev-infra/utils/git/index.ts | 13 ++++++++++--- 5 files changed, 33 insertions(+), 26 deletions(-) diff --git a/dev-infra/pr/discover-new-conflicts/index.ts b/dev-infra/pr/discover-new-conflicts/index.ts index daa1873092..790d809438 100644 --- a/dev-infra/pr/discover-new-conflicts/index.ts +++ b/dev-infra/pr/discover-new-conflicts/index.ts @@ -63,8 +63,8 @@ export async function discoverNewConflictsForPr( process.exit(1); } - /** The active github branch when the run began. */ - const originalBranch = git.getCurrentBranch(); + /** The active github branch or revision before we performed any Git commands. */ + const previousBranchOrRevision = git.getCurrentBranchOrRevision(); /* Progress bar to indicate progress. */ const progressBar = new Bar({format: `[{bar}] ETA: {eta}s | {value}/{total}`}); /* PRs which were found to be conflicting. */ @@ -103,7 +103,7 @@ export async function discoverNewConflictsForPr( const result = exec(`git rebase FETCH_HEAD`); if (result.code) { error('The requested PR currently has conflicts'); - cleanUpGitState(originalBranch); + cleanUpGitState(previousBranchOrRevision); process.exit(1); } @@ -130,7 +130,7 @@ export async function discoverNewConflictsForPr( info(); info(`Result:`); - cleanUpGitState(originalBranch); + cleanUpGitState(previousBranchOrRevision); // If no conflicts are found, exit successfully. if (conflicts.length === 0) { @@ -147,14 +147,14 @@ export async function discoverNewConflictsForPr( process.exit(1); } -/** Reset git back to the provided branch. */ -export function cleanUpGitState(branch: string) { +/** Reset git back to the provided branch or revision. */ +export function cleanUpGitState(previousBranchOrRevision: string) { // Ensure that any outstanding rebases are aborted. exec(`git rebase --abort`); // Ensure that any changes in the current repo state are cleared. exec(`git reset --hard`); // Checkout the original branch from before the run began. - exec(`git checkout ${branch}`); + exec(`git checkout ${previousBranchOrRevision}`); // Delete the generated branch. exec(`git branch -D ${tempWorkingBranch}`); } diff --git a/dev-infra/pr/merge/strategies/autosquash-merge.ts b/dev-infra/pr/merge/strategies/autosquash-merge.ts index 198eda3daf..e304ed98bd 100644 --- a/dev-infra/pr/merge/strategies/autosquash-merge.ts +++ b/dev-infra/pr/merge/strategies/autosquash-merge.ts @@ -59,7 +59,7 @@ export class AutosquashMergeStrategy extends MergeStrategy { // is desired, we set the `GIT_SEQUENCE_EDITOR` environment variable to `true` so that // the rebase seems interactive to Git, while it's not interactive to the user. // See: https://github.com/git/git/commit/891d4a0313edc03f7e2ecb96edec5d30dc182294. - const branchBeforeRebase = this.git.getCurrentBranch(); + const branchOrRevisionBeforeRebase = this.git.getCurrentBranchOrRevision(); const rebaseEnv = needsCommitMessageFixup ? undefined : {...process.env, GIT_SEQUENCE_EDITOR: 'true'}; this.git.run( @@ -69,9 +69,9 @@ export class AutosquashMergeStrategy extends MergeStrategy { // Update pull requests commits to reference the pull request. This matches what // Github does when pull requests are merged through the Web UI. The motivation is // that it should be easy to determine which pull request contained a given commit. - // **Note**: The filter-branch command relies on the working tree, so we want to make - // sure that we are on the initial branch where the merge script has been run. - this.git.run(['checkout', '-f', branchBeforeRebase]); + // Note: The filter-branch command relies on the working tree, so we want to make sure + // that we are on the initial branch or revision where the merge script has been invoked. + this.git.run(['checkout', '-f', branchOrRevisionBeforeRebase]); this.git.run( ['filter-branch', '-f', '--msg-filter', `${MSG_FILTER_SCRIPT} ${prNumber}`, revisionRange]); diff --git a/dev-infra/pr/merge/task.ts b/dev-infra/pr/merge/task.ts index e02815cbc0..5a17784738 100644 --- a/dev-infra/pr/merge/task.ts +++ b/dev-infra/pr/merge/task.ts @@ -87,14 +87,14 @@ export class PullRequestMergeTask { new GithubApiMergeStrategy(this.git, this.config.githubApiMerge) : new AutosquashMergeStrategy(this.git); - // Branch that is currently checked out so that we can switch back to it once - // the pull request has been merged. - let previousBranch: null|string = null; + // Branch or revision that is currently checked out so that we can switch back to + // it once the pull request has been merged. + let previousBranchOrRevision: null|string = null; // The following block runs Git commands as child processes. These Git commands can fail. // We want to capture these command errors and return an appropriate merge request status. try { - previousBranch = this.git.getCurrentBranch(); + previousBranchOrRevision = this.git.getCurrentBranchOrRevision(); // Run preparations for the merge (e.g. fetching branches). await strategy.prepare(pullRequest); @@ -107,7 +107,7 @@ export class PullRequestMergeTask { // Switch back to the previous branch. We need to do this before deleting the temporary // branches because we cannot delete branches which are currently checked out. - this.git.run(['checkout', '-f', previousBranch]); + this.git.run(['checkout', '-f', previousBranchOrRevision]); await strategy.cleanup(pullRequest); @@ -123,8 +123,8 @@ export class PullRequestMergeTask { } finally { // Always try to restore the branch if possible. We don't want to leave // the repository in a different state than before. - if (previousBranch !== null) { - this.git.runGraceful(['checkout', '-f', previousBranch]); + if (previousBranchOrRevision !== null) { + this.git.runGraceful(['checkout', '-f', previousBranchOrRevision]); } } } diff --git a/dev-infra/pr/rebase/index.ts b/dev-infra/pr/rebase/index.ts index 0b9fa9fe77..8c0af93324 100644 --- a/dev-infra/pr/rebase/index.ts +++ b/dev-infra/pr/rebase/index.ts @@ -50,10 +50,10 @@ export async function rebasePr( } /** - * The branch originally checked out before this method performs any Git - * operations that may change the working branch. + * The branch or revision originally checked out before this method performed + * any Git operations that may change the working branch. */ - const originalBranch = git.getCurrentBranch(); + const previousBranchOrRevision = git.getCurrentBranchOrRevision(); /* Get the PR information from Github. */ const pr = await getPr(PR_SCHEMA, prNumber, config.github); @@ -121,7 +121,7 @@ export async function rebasePr( info(); info(`To abort the rebase and return to the state of the repository before this command`); info(`run the following command:`); - info(` $ git rebase --abort && git reset --hard && git checkout ${originalBranch}`); + info(` $ git rebase --abort && git reset --hard && git checkout ${previousBranchOrRevision}`); process.exit(1); } else { info(`Cleaning up git state, and restoring previous state.`); @@ -137,7 +137,7 @@ export async function rebasePr( // Ensure that any changes in the current repo state are cleared. git.runGraceful(['reset', '--hard'], {stdio: 'ignore'}); // Checkout the original branch from before the run began. - git.runGraceful(['checkout', originalBranch], {stdio: 'ignore'}); + git.runGraceful(['checkout', previousBranchOrRevision], {stdio: 'ignore'}); } } diff --git a/dev-infra/utils/git/index.ts b/dev-infra/utils/git/index.ts index dbfb50c180..3a15b5a098 100644 --- a/dev-infra/utils/git/index.ts +++ b/dev-infra/utils/git/index.ts @@ -119,9 +119,16 @@ export class GitClient { return this.run(['branch', branchName, '--contains', sha]).stdout !== ''; } - /** Gets the currently checked out branch. */ - getCurrentBranch(): string { - return this.run(['rev-parse', '--abbrev-ref', 'HEAD']).stdout.trim(); + /** Gets the currently checked out branch or revision. */ + getCurrentBranchOrRevision(): string { + const branchName = this.run(['rev-parse', '--abbrev-ref', 'HEAD']).stdout.trim(); + // If no branch name could be resolved. i.e. `HEAD` has been returned, then Git + // is currently in a detached state. In those cases, we just want to return the + // currently checked out revision/SHA. + if (branchName === 'HEAD') { + return this.run(['rev-parse', 'HEAD']).stdout.trim(); + } + return branchName; } /** Gets whether the current Git repository has uncommitted changes. */ From 3ee666580ab0296d99faaffe4643eddcf564940d Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Thu, 25 Jun 2020 00:40:15 +0200 Subject: [PATCH 02/20] fix(dev-infra): merge script should not always require full repo permissions (#37718) We recently added OAuth scope checking to the dev-infra Git client and started leveraging it for the merge script. We set the `repo` scope as required for running the merge script. We can loosen this requirement as in the Angular org where the script is consumed, only pull requests on public repositories are merged through the script. This should help with reducing the risk with compromised tokens as no access had to be granted on `repo:invite`, `repo_deployment` etc. PR Close #37718 --- dev-infra/pr/merge/task.ts | 18 +++++++++++++----- dev-infra/utils/config.ts | 2 ++ dev-infra/utils/git/index.ts | 14 +++++++------- 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/dev-infra/pr/merge/task.ts b/dev-infra/pr/merge/task.ts index 5a17784738..7b9a76ead2 100644 --- a/dev-infra/pr/merge/task.ts +++ b/dev-infra/pr/merge/task.ts @@ -16,9 +16,6 @@ import {isPullRequest, loadAndValidatePullRequest,} from './pull-request'; import {GithubApiMergeStrategy} from './strategies/api-merge'; import {AutosquashMergeStrategy} from './strategies/autosquash-merge'; -/** Github OAuth scopes required for the merge task. */ -const REQUIRED_SCOPES = ['repo']; - /** Describes the status of a pull request merge. */ export const enum MergeStatus { UNKNOWN_GIT_ERROR, @@ -56,8 +53,19 @@ export class PullRequestMergeTask { * @param force Whether non-critical pull request failures should be ignored. */ async merge(prNumber: number, force = false): Promise { - // Assert the authenticated GitClient has access on the required scopes. - const hasOauthScopes = await this.git.hasOauthScopes(...REQUIRED_SCOPES); + // Check whether the given Github token has sufficient permissions for writing + // to the configured repository. If the repository is not private, only the + // reduced `public_repo` OAuth scope is sufficient for performing merges. + const hasOauthScopes = await this.git.hasOauthScopes((scopes, missing) => { + if (!scopes.includes('repo')) { + if (this.config.remote.private) { + missing.push('repo'); + } else if (!scopes.includes('public_repo')) { + missing.push('public_repo'); + } + } + }); + if (hasOauthScopes !== true) { return { status: MergeStatus.GITHUB_ERROR, diff --git a/dev-infra/utils/config.ts b/dev-infra/utils/config.ts index bcf6a82d60..bd08ee68e4 100644 --- a/dev-infra/utils/config.ts +++ b/dev-infra/utils/config.ts @@ -21,6 +21,8 @@ export interface GitClientConfig { name: string; /** If SSH protocol should be used for git interactions. */ useSsh?: boolean; + /** Whether the specified repository is private. */ + private?: boolean; } /** diff --git a/dev-infra/utils/git/index.ts b/dev-infra/utils/git/index.ts index 3a15b5a098..88c626dc20 100644 --- a/dev-infra/utils/git/index.ts +++ b/dev-infra/utils/git/index.ts @@ -21,6 +21,9 @@ type RateLimitResponseWithOAuthScopeHeader = Octokit.Response void; + /** Error for failed Git commands. */ export class GitCommandError extends Error { constructor(client: GitClient, public args: string[]) { @@ -155,14 +158,11 @@ export class GitClient { * Assert the GitClient instance is using a token with permissions for the all of the * provided OAuth scopes. */ - async hasOauthScopes(...requestedScopes: string[]): Promise { - const missingScopes: string[] = []; + async hasOauthScopes(testFn: OAuthScopeTestFunction): Promise { const scopes = await this.getAuthScopesForToken(); - requestedScopes.forEach(scope => { - if (!scopes.includes(scope)) { - missingScopes.push(scope); - } - }); + const missingScopes: string[] = []; + // Test Github OAuth scopes and collect missing ones. + testFn(scopes, missingScopes); // If no missing scopes are found, return true to indicate all OAuth Scopes are available. if (missingScopes.length === 0) { return true; From f954ab6f10cf9ad86e79ad72360b9ec9f95a0a85 Mon Sep 17 00:00:00 2001 From: Nick Hodges Date: Fri, 26 Jun 2020 06:08:18 -0400 Subject: [PATCH 03/20] docs: add note about the month being zero-based in the Date constructor (#37770) Because the month is zero based, it may confuse some users that '3' is in fact 'April'. This comment should clear that up. PR Close #37770 --- aio/content/examples/pipes/src/app/app.component.ts | 2 +- aio/content/examples/pipes/src/app/hero-birthday1.component.ts | 2 +- aio/content/examples/pipes/src/app/hero-birthday2.component.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/aio/content/examples/pipes/src/app/app.component.ts b/aio/content/examples/pipes/src/app/app.component.ts index 2b739ed2a0..5b71cc53c7 100644 --- a/aio/content/examples/pipes/src/app/app.component.ts +++ b/aio/content/examples/pipes/src/app/app.component.ts @@ -6,5 +6,5 @@ import { Component } from '@angular/core'; templateUrl: './app.component.html' }) export class AppComponent { - birthday = new Date(1988, 3, 15); // April 15, 1988 + birthday = new Date(1988, 3, 15); // April 15, 1988 -- since month parameter is zero-based } diff --git a/aio/content/examples/pipes/src/app/hero-birthday1.component.ts b/aio/content/examples/pipes/src/app/hero-birthday1.component.ts index d51914550f..4b475e80d6 100644 --- a/aio/content/examples/pipes/src/app/hero-birthday1.component.ts +++ b/aio/content/examples/pipes/src/app/hero-birthday1.component.ts @@ -8,5 +8,5 @@ import { Component } from '@angular/core'; // #enddocregion hero-birthday-template }) export class HeroBirthdayComponent { - birthday = new Date(1988, 3, 15); // April 15, 1988 + birthday = new Date(1988, 3, 15); // April 15, 1988 -- since month parameter is zero-based } diff --git a/aio/content/examples/pipes/src/app/hero-birthday2.component.ts b/aio/content/examples/pipes/src/app/hero-birthday2.component.ts index ce71c2ab1e..f8ee3ca911 100644 --- a/aio/content/examples/pipes/src/app/hero-birthday2.component.ts +++ b/aio/content/examples/pipes/src/app/hero-birthday2.component.ts @@ -12,7 +12,7 @@ import { Component } from '@angular/core'; }) // #docregion class export class HeroBirthday2Component { - birthday = new Date(1988, 3, 15); // April 15, 1988 + birthday = new Date(1988, 3, 15); // April 15, 1988 -- since month parameter is zero-based toggle = true; // start with true == shortDate get format() { return this.toggle ? 'shortDate' : 'fullDate'; } From 6341a837c1aaa51da694aa834cf6f212a1b64f14 Mon Sep 17 00:00:00 2001 From: Ajit Singh Date: Sat, 9 May 2020 16:54:03 +0530 Subject: [PATCH 04/20] docs: correct outdated dev instructions for public api golds (#37026) This change updates the dev instructions to reflect the location and generation of public API golds, which changed in #35768. PR Close #37026 --- docs/PUBLIC_API.md | 51 +++++++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/docs/PUBLIC_API.md b/docs/PUBLIC_API.md index be65edd1df..dc592c3788 100644 --- a/docs/PUBLIC_API.md +++ b/docs/PUBLIC_API.md @@ -53,40 +53,45 @@ If you modify any part of a public API in one of the supported public packages, The public API guard provides a Bazel target that updates the current status of a given package. If you add to or modify the public API in any way, you must use [yarn](https://yarnpkg.com/) to execute the Bazel target in your terminal shell of choice (a recent version of `bash` is recommended). ```shell -yarn bazel run //tools/public_api_guard:_api.accept +yarn bazel run //packages/:_api.accept ``` Using yarn ensures that you are running the correct version of Bazel. (Read more about building Angular with Bazel [here](./BAZEL.md).) -Here is an example of a Circle CI test failure that resulted from adding a new allowed type to a public property in `forms.d.ts`. Error messages from the API guard use [`git-diff` formatting](https://git-scm.com/docs/git-diff#_combined_diff_format). +Here is an example of a Circle CI test failure that resulted from adding a new allowed type to a public property in `core.d.ts`. Error messages from the API guard use [`git-diff` formatting](https://git-scm.com/docs/git-diff#_combined_diff_format). ``` -FAIL: //tools/public_api_guard:forms_api (see /home/circleci/.cache/bazel/_bazel_circleci/9ce5c2144ecf75d11717c0aa41e45a8d/execroot/angular/bazel-out/k8-fastbuild/testlogs/tools/public_api_guard/forms_api/test_attempts/attempt_1.log) -FAIL: //tools/public_api_guard:forms_api (see /home/circleci/.cache/bazel/_bazel_circleci/9ce5c2144ecf75d11717c0aa41e45a8d/execroot/angular/bazel-out/k8-fastbuild/testlogs/tools/public_api_guard/forms_api/test.log) +FAIL: //packages/core:core_api (see /home/circleci/.cache/bazel/_bazel_circleci/9ce5c2144ecf75d11717c0aa41e45a8d/execroot/angular/bazel-out/k8-fastbuild/testlogs/packages/core/core_api/test_attempts/attempt_1.log) +INFO: From Action packages/compiler-cli/ngcc/test/fesm5_angular_core.js: +[BABEL] Note: The code generator has deoptimised the styling of /b/f/w/bazel-out/k8-fastbuild/bin/packages/core/npm_package/fesm2015/core.js as it exceeds the max of 500KB. +FAIL: //packages/core:core_api (see /home/circleci/.cache/bazel/_bazel_circleci/9ce5c2144ecf75d11717c0aa41e45a8d/execroot/angular/bazel-out/k8-fastbuild/testlogs/packages/core/core_api/test.log) + +FAILED: //packages/core:core_api (Summary) + /home/circleci/.cache/bazel/_bazel_circleci/9ce5c2144ecf75d11717c0aa41e45a8d/execroot/angular/bazel-out/k8-fastbuild/testlogs/packages/core/core_api/test.log + /home/circleci/.cache/bazel/_bazel_circleci/9ce5c2144ecf75d11717c0aa41e45a8d/execroot/angular/bazel-out/k8-fastbuild/testlogs/packages/core/core_api/test_attempts/attempt_1.log +INFO: From Testing //packages/core:core_api: +==================== Test output for //packages/core:core_api: +/b/f/w/bazel-out/k8-fastbuild/bin/packages/core/core_api.sh.runfiles/angular/packages/core/npm_package/core.d.ts(7,1): error: No export declaration found for symbol "ComponentFactory" +--- goldens/public-api/core/core.d.ts Golden file ++++ goldens/public-api/core/core.d.ts Generated API +@@ -563,9 +563,9 @@ + ngModule: Type; + providers?: Provider[]; + } + +-export declare type NgIterable = Array | Iterable; ++export declare type NgIterable = Iterable; + + export declare interface NgModule { + bootstrap?: Array | any[]>; + declarations?: Array | any[]>; -FAILED: //tools/public_api_guard:forms_api (Summary) - /home/circleci/.cache/bazel/_bazel_circleci/9ce5c2144ecf75d11717c0aa41e45a8d/execroot/angular/bazel-out/k8-fastbuild/testlogs/tools/public_api_guard/forms_api/test.log - /home/circleci/.cache/bazel/_bazel_circleci/9ce5c2144ecf75d11717c0aa41e45a8d/execroot/angular/bazel-out/k8-fastbuild/testlogs/tools/public_api_guard/forms_api/test_attempts/attempt_1.log -INFO: From Testing //tools/public_api_guard:forms_api: -==================== Test output for //tools/public_api_guard:forms_api: ---- tools/public_api_guard/forms/forms.d.ts Golden file -+++ tools/public_api_guard/forms/forms.d.ts Generated API -@@ -4,9 +4,9 @@ - readonly disabled: boolean; - readonly enabled: boolean; - readonly errors: ValidationErrors | null; - readonly invalid: boolean; -- readonly parent: FormGroup | FormArray; -+ readonly parent: FormGroup | FormArray | undefined; - readonly pending: boolean; - readonly pristine: boolean; - readonly root: AbstractControl; - readonly status: string; If you modify a public API, you must accept the new golden file. To do so, execute the following Bazel target: - yarn bazel run //tools/public_api_guard:forms_api.accept + yarn bazel run //packages/core:core_api.accept + ``` From c942662d796112363d31a0c51fe06ace6f70d60b Mon Sep 17 00:00:00 2001 From: Keen Yee Liau Date: Thu, 25 Jun 2020 14:17:17 -0700 Subject: [PATCH 05/20] fix(language-service): reinstate getExternalFiles() (#37750) `getExternalFiles()` is an API that could optionally be provided by a tsserver plugin to notify the server of any additional files that should belong to a particular project. This API was removed in https://github.com/angular/angular/pull/34260 mainly due to performance reasons. However, with the introduction of "solution-style" tsconfig in typescript 3.9, the Angular extension could no longer reliably detect the owning Project solely based on the ancestor tsconfig.json. In order to support this use case, we have to reinstate `getExternalFiles()`. Fixes https://github.com/angular/vscode-ng-language-service/issues/824 PR Close #37750 --- packages/language-service/src/ts_plugin.ts | 25 ++++++++++++++++++- .../language-service/src/typescript_host.ts | 7 ++++++ .../language-service/test/ts_plugin_spec.ts | 9 ++++++- 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/packages/language-service/src/ts_plugin.ts b/packages/language-service/src/ts_plugin.ts index d0a3a5005f..05ad3c79c1 100644 --- a/packages/language-service/src/ts_plugin.ts +++ b/packages/language-service/src/ts_plugin.ts @@ -11,8 +11,30 @@ import * as tss from 'typescript/lib/tsserverlibrary'; import {createLanguageService} from './language_service'; import {TypeScriptServiceHost} from './typescript_host'; +// Use a WeakMap to keep track of Project to Host mapping so that when Project +// is deleted Host could be garbage collected. +const PROJECT_MAP = new WeakMap(); + +/** + * This function is called by tsserver to retrieve the external (non-TS) files + * that should belong to the specified `project`. For Angular, these files are + * external templates. This is called once when the project is loaded, then + * every time when the program is updated. + * @param project Project for which external files should be retrieved. + */ +export function getExternalFiles(project: tss.server.Project): string[] { + if (!project.hasRoots()) { + // During project initialization where there is no root files yet we should + // not do any work. + return []; + } + const ngLsHost = PROJECT_MAP.get(project); + ngLsHost?.getAnalyzedModules(); + return ngLsHost?.getExternalTemplates() || []; +} + export function create(info: tss.server.PluginCreateInfo): tss.LanguageService { - const {languageService: tsLS, languageServiceHost: tsLSHost, config} = info; + const {languageService: tsLS, languageServiceHost: tsLSHost, config, project} = info; // This plugin could operate under two different modes: // 1. TS + Angular // Plugin augments TS language service to provide additional Angular @@ -25,6 +47,7 @@ export function create(info: tss.server.PluginCreateInfo): tss.LanguageService { const angularOnly = config ? config.angularOnly === true : false; const ngLSHost = new TypeScriptServiceHost(tsLSHost, tsLS); const ngLS = createLanguageService(ngLSHost); + PROJECT_MAP.set(project, ngLSHost); function getCompletionsAtPosition( fileName: string, position: number, options: tss.GetCompletionsAtPositionOptions|undefined) { diff --git a/packages/language-service/src/typescript_host.ts b/packages/language-service/src/typescript_host.ts index cf4bf437b0..ae927fd77d 100644 --- a/packages/language-service/src/typescript_host.ts +++ b/packages/language-service/src/typescript_host.ts @@ -151,6 +151,13 @@ export class TypeScriptServiceHost implements LanguageServiceHost { return this.resolver.getReflector() as StaticReflector; } + /** + * Return all known external templates. + */ + getExternalTemplates(): string[] { + return [...this.fileToComponent.keys()]; + } + /** * Checks whether the program has changed and returns all analyzed modules. * If program has changed, invalidate all caches and update fileToComponent diff --git a/packages/language-service/test/ts_plugin_spec.ts b/packages/language-service/test/ts_plugin_spec.ts index eecf101257..dc82a9cf58 100644 --- a/packages/language-service/test/ts_plugin_spec.ts +++ b/packages/language-service/test/ts_plugin_spec.ts @@ -8,7 +8,7 @@ import * as ts from 'typescript'; -import {create} from '../src/ts_plugin'; +import {create, getExternalFiles} from '../src/ts_plugin'; import {CompletionKind} from '../src/types'; import {MockTypescriptHost} from './test_utils'; @@ -129,6 +129,13 @@ describe('plugin', () => { }, ]); }); + + it('should return external templates when getExternalFiles() is called', () => { + const externalTemplates = getExternalFiles(mockProject); + expect(externalTemplates).toEqual([ + '/app/test.ng', + ]); + }); }); describe(`with config 'angularOnly = true`, () => { From e0eeb4afcb68129c9b5079cde4274a2f0af741d6 Mon Sep 17 00:00:00 2001 From: Keen Yee Liau Date: Tue, 12 May 2020 13:23:05 -0700 Subject: [PATCH 06/20] refactor(compiler-cli): Remove any cast for CompilerHost (#37079) This commit removes the FIXME for casting CompilerHost to any since google3 is now already on TS 3.8. PR Close #37079 --- packages/compiler-cli/src/ngtsc/util/src/typescript.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/util/src/typescript.ts b/packages/compiler-cli/src/ngtsc/util/src/typescript.ts index ee64a577ec..13a90d3a3e 100644 --- a/packages/compiler-cli/src/ngtsc/util/src/typescript.ts +++ b/packages/compiler-cli/src/ngtsc/util/src/typescript.ts @@ -125,10 +125,11 @@ export function resolveModuleName( compilerHost: ts.ModuleResolutionHost&Pick, moduleResolutionCache: ts.ModuleResolutionCache|null): ts.ResolvedModule|undefined { if (compilerHost.resolveModuleNames) { - // FIXME: Additional parameters are required in TS3.6, but ignored in 3.5. - // Remove the any cast once google3 is fully on TS3.6. - return (compilerHost as any) - .resolveModuleNames([moduleName], containingFile, undefined, undefined, compilerOptions)[0]; + return compilerHost.resolveModuleNames( + [moduleName], containingFile, + undefined, // reusedNames + undefined, // redirectedReference + compilerOptions)[0]; } else { return ts .resolveModuleName( From 83fe963a4b927341f385b98d7bf27f562f9b4f33 Mon Sep 17 00:00:00 2001 From: Joey Perrott Date: Wed, 17 Jun 2020 05:34:41 -0700 Subject: [PATCH 07/20] build: move shims_for_IE to third_party directory (#37624) The shims_for_IE.js file contains vendor code that predates the third_party directory. This file is currently used for internal karma testing setup. This change corrects this by moving the shims_for_IE file to //third_part/ PR Close #37624 --- .ng-dev/config.ts | 2 -- BUILD.bazel | 2 +- karma-js.conf.js | 2 +- packages/router/karma.conf.js | 2 +- shims_for_IE.js => third_party/shims_for_IE.js | 0 5 files changed, 3 insertions(+), 5 deletions(-) rename shims_for_IE.js => third_party/shims_for_IE.js (100%) diff --git a/.ng-dev/config.ts b/.ng-dev/config.ts index 6350332d5e..359304fadb 100644 --- a/.ng-dev/config.ts +++ b/.ng-dev/config.ts @@ -56,8 +56,6 @@ const format = { // TODO: burn down format failures and remove aio and integration exceptions. '!aio/**', '!integration/**', - // TODO: remove this exclusion as part of IE deprecation. - '!shims_for_IE.js', // Both third_party and .yarn are directories containing copied code which should // not be modified. '!third_party/**', diff --git a/BUILD.bazel b/BUILD.bazel index 48e84cc328..73e67bea63 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -24,7 +24,7 @@ filegroup( "//packages/zone.js/bundles:zone-testing.umd.js", "//packages/zone.js/bundles:task-tracking.umd.js", "//:test-events.js", - "//:shims_for_IE.js", + "//:third_party/shims_for_IE.js", # Including systemjs because it defines `__eval`, which produces correct stack traces. "@npm//:node_modules/systemjs/dist/system.src.js", "@npm//:node_modules/reflect-metadata/Reflect.js", diff --git a/karma-js.conf.js b/karma-js.conf.js index ce31b84a83..bdab6ea463 100644 --- a/karma-js.conf.js +++ b/karma-js.conf.js @@ -43,7 +43,7 @@ module.exports = function(config) { // Including systemjs because it defines `__eval`, which produces correct stack traces. 'test-events.js', - 'shims_for_IE.js', + 'third_party/shims_for_IE.js', 'node_modules/systemjs/dist/system.src.js', // Serve polyfills necessary for testing the `elements` package. diff --git a/packages/router/karma.conf.js b/packages/router/karma.conf.js index 038d8e2654..2b5cb9e95a 100644 --- a/packages/router/karma.conf.js +++ b/packages/router/karma.conf.js @@ -28,7 +28,7 @@ module.exports = function(config) { // Polyfills. 'node_modules/core-js/client/core.js', 'node_modules/reflect-metadata/Reflect.js', - 'shims_for_IE.js', + 'third_party/shims_for_IE.js', // System.js for module loading 'node_modules/systemjs/dist/system-polyfills.js', diff --git a/shims_for_IE.js b/third_party/shims_for_IE.js similarity index 100% rename from shims_for_IE.js rename to third_party/shims_for_IE.js From 0879d2e85d29c33c10e7314e49f5e4f0cc6ce2ba Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Fri, 6 Mar 2020 11:57:29 -0800 Subject: [PATCH 08/20] refactor(core): throw more descriptive error message in case of invalid host element (#35916) This commit replaces an assert with more descriptive error message that is thrown in case `` or `` is used as host element for a Component. Resolves #35240. PR Close #35916 --- .../size-tracking/integration-payloads.json | 2 +- packages/core/src/render3/component_ref.ts | 2 +- packages/core/src/render3/di.ts | 2 +- packages/core/src/render3/instructions/di.ts | 3 +- .../core/src/render3/instructions/listener.ts | 2 +- .../core/src/render3/instructions/shared.ts | 13 +++- packages/core/src/render3/node_assert.ts | 6 +- .../core/src/render3/node_manipulation.ts | 20 +++--- packages/core/src/render3/query.ts | 2 +- .../src/render3/view_engine_compatibility.ts | 2 +- packages/core/src/render3/view_ref.ts | 8 +-- .../core/test/acceptance/component_spec.ts | 61 ++++++++++++++++++- 12 files changed, 95 insertions(+), 28 deletions(-) diff --git a/goldens/size-tracking/integration-payloads.json b/goldens/size-tracking/integration-payloads.json index bc40e78b62..bf9cd1e47f 100644 --- a/goldens/size-tracking/integration-payloads.json +++ b/goldens/size-tracking/integration-payloads.json @@ -62,7 +62,7 @@ "bundle": "TODO(i): we should define ngDevMode to false in Closure, but --define only works in the global scope.", "bundle": "TODO(i): (FW-2164) TS 3.9 new class shape seems to have broken Closure in big ways. The size went from 169991 to 252338", "bundle": "TODO(i): after removal of tsickle from ngc-wrapped / ng_package, we had to switch to SIMPLE optimizations which increased the size from 252338 to 1198917, see PR#37221 and PR#37317 for more info", - "bundle": 1209688 + "bundle": 1210239 } } } diff --git a/packages/core/src/render3/component_ref.ts b/packages/core/src/render3/component_ref.ts index 5c8e8b1ae5..eed3fb24d6 100644 --- a/packages/core/src/render3/component_ref.ts +++ b/packages/core/src/render3/component_ref.ts @@ -225,7 +225,7 @@ export class ComponentFactory extends viewEngine_ComponentFactory { createElementRef(viewEngine_ElementRef, tElementNode, rootLView), rootLView, tElementNode); // The host element of the internal root view is attached to the component's host view node. - ngDevMode && assertNodeOfPossibleTypes(rootTView.node, TNodeType.View); + ngDevMode && assertNodeOfPossibleTypes(rootTView.node, [TNodeType.View]); rootTView.node!.child = tElementNode; return componentRef; diff --git a/packages/core/src/render3/di.ts b/packages/core/src/render3/di.ts index 0ce15a35ad..a80b1d185d 100644 --- a/packages/core/src/render3/di.ts +++ b/packages/core/src/render3/di.ts @@ -267,7 +267,7 @@ export function diPublicInInjector( export function injectAttributeImpl(tNode: TNode, attrNameToInject: string): string|null { ngDevMode && assertNodeOfPossibleTypes( - tNode, TNodeType.Container, TNodeType.Element, TNodeType.ElementContainer); + tNode, [TNodeType.Container, TNodeType.Element, TNodeType.ElementContainer]); ngDevMode && assertDefined(tNode, 'expecting tNode'); if (attrNameToInject === 'class') { return tNode.classes; diff --git a/packages/core/src/render3/instructions/di.ts b/packages/core/src/render3/instructions/di.ts index ce4110fe21..075ed38037 100644 --- a/packages/core/src/render3/instructions/di.ts +++ b/packages/core/src/render3/instructions/di.ts @@ -9,8 +9,7 @@ import {InjectFlags, InjectionToken, resolveForwardRef} from '../../di'; import {ɵɵinject} from '../../di/injector_compatibility'; import {Type} from '../../interface/type'; import {getOrCreateInjectable, injectAttributeImpl} from '../di'; -import {TDirectiveHostNode, TNodeType} from '../interfaces/node'; -import {assertNodeOfPossibleTypes} from '../node_assert'; +import {TDirectiveHostNode} from '../interfaces/node'; import {getLView, getPreviousOrParentTNode} from '../state'; /** diff --git a/packages/core/src/render3/instructions/listener.ts b/packages/core/src/render3/instructions/listener.ts index 0ed0540288..3a8daffc70 100644 --- a/packages/core/src/render3/instructions/listener.ts +++ b/packages/core/src/render3/instructions/listener.ts @@ -128,7 +128,7 @@ function listenerInternal( ngDevMode && assertNodeOfPossibleTypes( - tNode, TNodeType.Element, TNodeType.Container, TNodeType.ElementContainer); + tNode, [TNodeType.Element, TNodeType.Container, TNodeType.ElementContainer]); let processOutputs = true; diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index 1ca8a714d7..b80e445cf5 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -15,6 +15,7 @@ import {assertDataInRange, assertDefined, assertDomNode, assertEqual, assertGrea import {createNamedArrayType} from '../../util/named_array_type'; import {initNgDevMode} from '../../util/ng_dev_mode'; import {normalizeDebugBindingName, normalizeDebugBindingValue} from '../../util/ng_reflect'; +import {stringify} from '../../util/stringify'; import {assertFirstCreatePass, assertLContainer, assertLView} from '../assert'; import {attachPatchData} from '../context_discovery'; import {getFactoryDef} from '../definition'; @@ -272,7 +273,7 @@ export function assignTViewNodeToLView( let tNode = tView.node; if (tNode == null) { ngDevMode && tParentNode && - assertNodeOfPossibleTypes(tParentNode, TNodeType.Element, TNodeType.Container); + assertNodeOfPossibleTypes(tParentNode, [TNodeType.Element, TNodeType.Container]); tView.node = tNode = createTNode( tView, tParentNode as TElementNode | TContainerNode | null, // @@ -1278,7 +1279,7 @@ function instantiateAllDirectives( const isComponent = isComponentDef(def); if (isComponent) { - ngDevMode && assertNodeOfPossibleTypes(tNode, TNodeType.Element); + ngDevMode && assertNodeOfPossibleTypes(tNode, [TNodeType.Element]); addComponentLogic(lView, tNode as TElementNode, def as ComponentDef); } @@ -1366,7 +1367,7 @@ function findDirectiveDefMatches( ngDevMode && assertFirstCreatePass(tView); ngDevMode && assertNodeOfPossibleTypes( - tNode, TNodeType.Element, TNodeType.ElementContainer, TNodeType.Container); + tNode, [TNodeType.Element, TNodeType.ElementContainer, TNodeType.Container]); const registry = tView.directiveRegistry; let matches: any[]|null = null; if (registry) { @@ -1377,6 +1378,12 @@ function findDirectiveDefMatches( diPublicInInjector(getOrCreateNodeInjectorForNode(tNode, viewData), tView, def.type); if (isComponentDef(def)) { + ngDevMode && + assertNodeOfPossibleTypes( + tNode, [TNodeType.Element], + `"${tNode.tagName}" tags cannot be used as component hosts. ` + + `Please use a different tag to activate the ${ + stringify(def.type)} component.`); if (tNode.flags & TNodeFlags.isComponentHost) throwMultipleComponentError(tNode); markAsComponentHost(tView, tNode); // The component is always stored first with directives after. diff --git a/packages/core/src/render3/node_assert.ts b/packages/core/src/render3/node_assert.ts index 895c52397d..59714e88df 100644 --- a/packages/core/src/render3/node_assert.ts +++ b/packages/core/src/render3/node_assert.ts @@ -26,12 +26,14 @@ export function assertNodeType(tNode: TNode, type: TNodeType): asserts tNode is assertEqual(tNode.type, type, `should be a ${typeName(type)}`); } -export function assertNodeOfPossibleTypes(tNode: TNode|null, ...types: TNodeType[]): void { +export function assertNodeOfPossibleTypes( + tNode: TNode|null, types: TNodeType[], message?: string): void { assertDefined(tNode, 'should be called with a TNode'); const found = types.some(type => tNode.type === type); assertEqual( found, true, - `Should be one of ${types.map(typeName).join(', ')} but got ${typeName(tNode.type)}`); + message ?? + `Should be one of ${types.map(typeName).join(', ')} but got ${typeName(tNode.type)}`); } export function assertNodeNotOfTypes(tNode: TNode, types: TNodeType[], message?: string): void { diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index 4717bb70fd..c7ce0d2542 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -552,7 +552,7 @@ function getRenderParent(tView: TView, tNode: TNode, currentView: LView): REleme } else { // We are inserting a root element of the component view into the component host element and // it should always be eager. - ngDevMode && assertNodeOfPossibleTypes(hostTNode, TNodeType.Element); + ngDevMode && assertNodeOfPossibleTypes(hostTNode, [TNodeType.Element]); return currentView[HOST]; } } else { @@ -698,10 +698,10 @@ export function appendChild( */ function getFirstNativeNode(lView: LView, tNode: TNode|null): RNode|null { if (tNode !== null) { - ngDevMode && - assertNodeOfPossibleTypes( - tNode, TNodeType.Element, TNodeType.Container, TNodeType.ElementContainer, - TNodeType.IcuContainer, TNodeType.Projection); + ngDevMode && assertNodeOfPossibleTypes(tNode, [ + TNodeType.Element, TNodeType.Container, TNodeType.ElementContainer, TNodeType.IcuContainer, + TNodeType.Projection + ]); const tNodeType = tNode.type; if (tNodeType === TNodeType.Element) { @@ -778,10 +778,10 @@ function applyNodes( renderParent: RElement|null, beforeNode: RNode|null, isProjection: boolean) { while (tNode != null) { ngDevMode && assertTNodeForLView(tNode, lView); - ngDevMode && - assertNodeOfPossibleTypes( - tNode, TNodeType.Container, TNodeType.Element, TNodeType.ElementContainer, - TNodeType.Projection, TNodeType.Projection, TNodeType.IcuContainer); + ngDevMode && assertNodeOfPossibleTypes(tNode, [ + TNodeType.Container, TNodeType.Element, TNodeType.ElementContainer, TNodeType.Projection, + TNodeType.IcuContainer + ]); const rawSlotValue = lView[tNode.index]; const tNodeType = tNode.type; if (isProjection) { @@ -798,7 +798,7 @@ function applyNodes( applyProjectionRecursive( renderer, action, lView, tNode as TProjectionNode, renderParent, beforeNode); } else { - ngDevMode && assertNodeOfPossibleTypes(tNode, TNodeType.Element, TNodeType.Container); + ngDevMode && assertNodeOfPossibleTypes(tNode, [TNodeType.Element, TNodeType.Container]); applyToElementOrContainer(action, renderer, renderParent, rawSlotValue, beforeNode); } } diff --git a/packages/core/src/render3/query.ts b/packages/core/src/render3/query.ts index 0e73f530db..63eaf2bd7d 100644 --- a/packages/core/src/render3/query.ts +++ b/packages/core/src/render3/query.ts @@ -326,7 +326,7 @@ function createSpecialToken(lView: LView, tNode: TNode, read: any): any { } else if (read === ViewContainerRef) { ngDevMode && assertNodeOfPossibleTypes( - tNode, TNodeType.Element, TNodeType.Container, TNodeType.ElementContainer); + tNode, [TNodeType.Element, TNodeType.Container, TNodeType.ElementContainer]); return createContainerRef( ViewContainerRef, ViewEngine_ElementRef, tNode as TElementNode | TContainerNode | TElementContainerNode, lView); diff --git a/packages/core/src/render3/view_engine_compatibility.ts b/packages/core/src/render3/view_engine_compatibility.ts index f39721f7e4..42e69cea5a 100644 --- a/packages/core/src/render3/view_engine_compatibility.ts +++ b/packages/core/src/render3/view_engine_compatibility.ts @@ -340,7 +340,7 @@ export function createContainerRef( ngDevMode && assertNodeOfPossibleTypes( - hostTNode, TNodeType.Container, TNodeType.Element, TNodeType.ElementContainer); + hostTNode, [TNodeType.Container, TNodeType.Element, TNodeType.ElementContainer]); let lContainer: LContainer; const slotValue = hostView[hostTNode.index]; diff --git a/packages/core/src/render3/view_ref.ts b/packages/core/src/render3/view_ref.ts index 4da248498a..3421398726 100644 --- a/packages/core/src/render3/view_ref.ts +++ b/packages/core/src/render3/view_ref.ts @@ -324,10 +324,10 @@ function collectNativeNodes( tView: TView, lView: LView, tNode: TNode|null, result: any[], isProjection: boolean = false): any[] { while (tNode !== null) { - ngDevMode && - assertNodeOfPossibleTypes( - tNode, TNodeType.Element, TNodeType.Container, TNodeType.Projection, - TNodeType.ElementContainer, TNodeType.IcuContainer); + ngDevMode && assertNodeOfPossibleTypes(tNode, [ + TNodeType.Element, TNodeType.Container, TNodeType.Projection, TNodeType.ElementContainer, + TNodeType.IcuContainer + ]); const lNode = lView[tNode.index]; if (lNode !== null) { diff --git a/packages/core/test/acceptance/component_spec.ts b/packages/core/test/acceptance/component_spec.ts index ae3e605f06..46ed24bdfa 100644 --- a/packages/core/test/acceptance/component_spec.ts +++ b/packages/core/test/acceptance/component_spec.ts @@ -11,7 +11,7 @@ import {ApplicationRef, Component, ComponentFactoryResolver, ComponentRef, Eleme import {TestBed} from '@angular/core/testing'; import {ɵDomRendererFactory2 as DomRendererFactory2} from '@angular/platform-browser'; import {expect} from '@angular/platform-browser/testing/src/matchers'; -import {onlyInIvy} from '@angular/private/testing'; +import {ivyEnabled, onlyInIvy} from '@angular/private/testing'; import {domRendererFactory3} from '../../src/render3/interfaces/renderer'; @@ -259,6 +259,65 @@ describe('component', () => { expect(wrapperEls.length).toBe(2); // other elements are preserved }); + describe('invalid host element', () => { + it('should throw when is used as a host element for a Component', () => { + @Component({ + selector: 'ng-container', + template: '...', + }) + class Comp { + } + + @Component({ + selector: 'root', + template: '', + }) + class App { + } + + TestBed.configureTestingModule({declarations: [App, Comp]}); + if (ivyEnabled) { + expect(() => TestBed.createComponent(App)) + .toThrowError( + /"ng-container" tags cannot be used as component hosts. Please use a different tag to activate the Comp component/); + } else { + // In VE there is no special check for the case when `` is used as a host + // element for a Component. VE tries to attach Component's content to a Comment node that + // represents the `` location and this call fails with a + // browser/environment-specific error message, so we just verify that this scenario is + // triggering an error in VE. + expect(() => TestBed.createComponent(App)).toThrow(); + } + }); + + it('should throw when is used as a host element for a Component', () => { + @Component({ + selector: 'ng-template', + template: '...', + }) + class Comp { + } + + @Component({ + selector: 'root', + template: '', + }) + class App { + } + + TestBed.configureTestingModule({declarations: [App, Comp]}); + if (ivyEnabled) { + expect(() => TestBed.createComponent(App)) + .toThrowError( + /"ng-template" tags cannot be used as component hosts. Please use a different tag to activate the Comp component/); + } else { + expect(() => TestBed.createComponent(App)) + .toThrowError( + /Components on an embedded template: Comp \("\[ERROR ->\]<\/ng-template>"\)/); + } + }); + }); + it('should use a new ngcontent attribute for child elements created w/ Renderer2', () => { @Component({ selector: 'app-root', From fc5c34d1b804e062c3305458e1167aa8dc3591ca Mon Sep 17 00:00:00 2001 From: Santosh Yadav Date: Thu, 25 Jun 2020 07:01:32 +0530 Subject: [PATCH 09/20] feat(platform-browser): Allow `sms`-URLs (#31463) sms ulr is already supported by google/closure-library and and validations are added to check if the body passed is safe or not you can refer https://github.com/google/closure-library/blob/bb7ea653194b590711614fd79b0a88a38a600740/closure/goog/html/safeurl.js#L440-L454 for more details Fixes #31462 PR Close #31463 --- packages/core/src/sanitization/url_sanitizer.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/sanitization/url_sanitizer.ts b/packages/core/src/sanitization/url_sanitizer.ts index 9111a5df1b..c3f91fc51e 100644 --- a/packages/core/src/sanitization/url_sanitizer.ts +++ b/packages/core/src/sanitization/url_sanitizer.ts @@ -34,7 +34,7 @@ import {isDevMode} from '../util/is_dev_mode'; * * This regular expression was taken from the Closure sanitization library. */ -const SAFE_URL_PATTERN = /^(?:(?:https?|mailto|ftp|tel|file):|[^&:/?#]*(?:[/?#]|$))/gi; +const SAFE_URL_PATTERN = /^(?:(?:https?|mailto|ftp|tel|file|sms):|[^&:/?#]*(?:[/?#]|$))/gi; /* A pattern that matches safe srcset values */ const SAFE_SRCSET_PATTERN = /^(?:(?:https?|file):|[^&:/?#]*(?:[/?#]|$))/gi; From c5b125b7db50914840849a8d86cbb3304d2f4e68 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Thu, 25 Jun 2020 17:39:55 -0700 Subject: [PATCH 10/20] feat(dev-infra): add support for minBodyLengthTypeExcludes to commit-message validation (#37764) This feature will allow us to exclude certain commits from the 100 chars minBodyLength requirement for commit messages which is hard to satisfy for commits that make trivial changes (e.g. fixing typos in docs or comments). PR Close #37764 --- dev-infra/commit-message/config.ts | 3 +- dev-infra/commit-message/validate.spec.ts | 52 ++++++++++++++++++++--- dev-infra/commit-message/validate.ts | 5 ++- 3 files changed, 51 insertions(+), 9 deletions(-) diff --git a/dev-infra/commit-message/config.ts b/dev-infra/commit-message/config.ts index 7a84edc488..2d9739cecf 100644 --- a/dev-infra/commit-message/config.ts +++ b/dev-infra/commit-message/config.ts @@ -11,6 +11,7 @@ import {assertNoErrors, getConfig, NgDevConfig} from '../utils/config'; export interface CommitMessageConfig { maxLineLength: number; minBodyLength: number; + minBodyLengthTypeExcludes?: string[]; types: string[]; scopes: string[]; } @@ -19,7 +20,7 @@ export interface CommitMessageConfig { export function getCommitMessageConfig() { // List of errors encountered validating the config. const errors: string[] = []; - // The unvalidated config object. + // The non-validated config object. const config: Partial> = getConfig(); if (config.commitMessage === undefined) { diff --git a/dev-infra/commit-message/validate.spec.ts b/dev-infra/commit-message/validate.spec.ts index e745fa7158..ad1bf5b1e9 100644 --- a/dev-infra/commit-message/validate.spec.ts +++ b/dev-infra/commit-message/validate.spec.ts @@ -10,19 +10,22 @@ import * as validateConfig from './config'; import {validateCommitMessage} from './validate'; +type CommitMessageConfig = validateConfig.CommitMessageConfig; + + // Constants -const config = { - 'commitMessage': { - 'maxLineLength': 120, - 'minBodyLength': 0, - 'types': [ +const config: {commitMessage: CommitMessageConfig} = { + commitMessage: { + maxLineLength: 120, + minBodyLength: 0, + types: [ 'feat', 'fix', 'refactor', 'release', 'style', ], - 'scopes': [ + scopes: [ 'common', 'compiler', 'core', @@ -224,5 +227,42 @@ describe('validate-commit-message.js', () => { }); }); }); + + describe('minBodyLength', () => { + const minBodyLengthConfig: {commitMessage: CommitMessageConfig} = { + commitMessage: { + maxLineLength: 120, + minBodyLength: 30, + minBodyLengthTypeExcludes: ['docs'], + types: ['fix', 'docs'], + scopes: ['core'] + } + }; + + beforeEach(() => { + (validateConfig.getCommitMessageConfig as jasmine.Spy).and.returnValue(minBodyLengthConfig); + }); + + it('should fail validation if the body is shorter than `minBodyLength`', () => { + expect(validateCommitMessage( + 'fix(core): something\n\n Explanation of the motivation behind this change')) + .toBe(VALID); + expect(validateCommitMessage('fix(core): something\n\n too short')).toBe(INVALID); + expect(lastError).toContain( + 'The commit message body does not meet the minimum length of 30 characters'); + expect(validateCommitMessage('fix(core): something')).toBe(INVALID); + expect(lastError).toContain( + 'The commit message body does not meet the minimum length of 30 characters'); + }); + + it('should pass validation if the body is shorter than `minBodyLength` but the commit type is in the `minBodyLengthTypeExclusions` list', + () => { + expect(validateCommitMessage('docs: just fixing a typo')).toBe(VALID); + expect(validateCommitMessage('docs(core): just fixing a typo')).toBe(VALID); + expect(validateCommitMessage( + 'docs(core): just fixing a typo\n\nThis was just a silly typo.')) + .toBe(VALID); + }); + }); }); }); diff --git a/dev-infra/commit-message/validate.ts b/dev-infra/commit-message/validate.ts index 6148f6dfe2..a9b01f1c4d 100644 --- a/dev-infra/commit-message/validate.ts +++ b/dev-infra/commit-message/validate.ts @@ -148,7 +148,8 @@ export function validateCommitMessage( // Checking commit body // ////////////////////////// - if (commit.bodyWithoutLinking.trim().length < config.minBodyLength) { + if (!config.minBodyLengthTypeExcludes?.includes(commit.type) && + commit.bodyWithoutLinking.trim().length < config.minBodyLength) { printError(`The commit message body does not meet the minimum length of ${ config.minBodyLength} characters`); return false; @@ -157,7 +158,7 @@ export function validateCommitMessage( const bodyByLine = commit.body.split('\n'); if (bodyByLine.some(line => line.length > config.maxLineLength)) { printError( - `The commit messsage body contains lines greater than ${config.maxLineLength} characters`); + `The commit message body contains lines greater than ${config.maxLineLength} characters`); return false; } From acf3cff9eedbcfbc4476e0597d0a3c7a883bd05f Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Thu, 25 Jun 2020 17:45:06 -0700 Subject: [PATCH 11/20] ci: exclude "docs" commit type from minBodyLength commit message validation (#37764) docs commits are sometimes trivial (e.g. an obvious typo fix) and in such cases its very akward to to write up 100 chars worth of text about why this typo fix is the best thing in the world and why it is so important and crucial that we must know why we are fixing the typo at all. After all most typos are not just typos. Or are they? We'll shall see... PR Close #37764 --- .ng-dev/config.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/.ng-dev/config.ts b/.ng-dev/config.ts index 359304fadb..342b3f64f7 100644 --- a/.ng-dev/config.ts +++ b/.ng-dev/config.ts @@ -4,6 +4,7 @@ import {MergeConfig} from '../dev-infra/pr/merge/config'; const commitMessage = { 'maxLength': 120, 'minBodyLength': 100, + 'minBodyLengthExcludes': ['docs'], 'types': [ 'build', 'ci', From 8fd8143ab80470defb8c366ad4ebf6f9bece155d Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Fri, 26 Jun 2020 13:35:40 -0700 Subject: [PATCH 12/20] docs: release notes for the v10.0.1 release --- CHANGELOG.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f7f4739d4d..531ca66972 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,26 @@ + +## [10.0.1](https://github.com/angular/angular/compare/10.0.0...10.0.1) (2020-06-26) + + +### Bug Fixes + +* **core:** cleanup DOM elements when root view is removed ([#37600](https://github.com/angular/angular/issues/37600)) ([64f2ffa](https://github.com/angular/angular/commit/64f2ffa)), closes [#36449](https://github.com/angular/angular/issues/36449) +* **forms:** change error message ([#37643](https://github.com/angular/angular/issues/37643)) ([c5bc2e7](https://github.com/angular/angular/commit/c5bc2e7)) +* **forms:** correct usage of `selectedOptions` ([#37620](https://github.com/angular/angular/issues/37620)) ([dfb58c4](https://github.com/angular/angular/commit/dfb58c4)), closes [#37433](https://github.com/angular/angular/issues/37433) +* **http:** avoid abort a request when fetch operation is completed ([#37367](https://github.com/angular/angular/issues/37367)) ([a5d5f67](https://github.com/angular/angular/commit/a5d5f67)) +* **language-service:** reinstate getExternalFiles() ([#37750](https://github.com/angular/angular/issues/37750)) ([ad6680f](https://github.com/angular/angular/commit/ad6680f)) +* **migrations:** do not incorrectly add todo for @Injectable or @Pipe ([#37732](https://github.com/angular/angular/issues/37732)) ([13020b9](https://github.com/angular/angular/commit/13020b9)), closes [#37726](https://github.com/angular/angular/issues/37726) +* **router:** `RouterLinkActive` should run CD when setting `isActive` ([#21411](https://github.com/angular/angular/issues/21411)) ([a8ea817](https://github.com/angular/angular/commit/a8ea817)), closes [#15943](https://github.com/angular/angular/issues/15943) [#19934](https://github.com/angular/angular/issues/19934) +* **router:** add null support for RouterLink directive ([#32616](https://github.com/angular/angular/issues/32616)) ([69948ce](https://github.com/angular/angular/commit/69948ce)) +* **router:** fix error when calling ParamMap.get function ([#31599](https://github.com/angular/angular/issues/31599)) ([3190ccf](https://github.com/angular/angular/commit/3190ccf)) + + +### Performance Improvements + +* **compiler-cli:** fix regressions in incremental program reuse ([#37690](https://github.com/angular/angular/issues/37690)) ([96b96fb](https://github.com/angular/angular/commit/96b96fb)) + + + # [10.0.0](https://github.com/angular/angular/compare/10.0.0-rc.6...10.0.0) (2020-06-24) From ae5257cda6dd6957c3def575176e922111ea170d Mon Sep 17 00:00:00 2001 From: Keen Yee Liau Date: Tue, 9 Jun 2020 16:46:20 -0700 Subject: [PATCH 13/20] fix(language-service): incorrect autocomplete results on unknown symbol (#37518) This commit fixes a bug whereby the language service would incorrectly return HTML elements if autocomplete is requested for an unknown symbol. This is because we walk through every possible scenario, and fallback to element autocomplete if none of the scenarios match. The fix here is to return results from interpolation if we know for sure we are in a bound text. This means we will now return an empty results if there is no suggestions. This commit also refactors the code a little to make it easier to understand. PR Close #37518 --- packages/language-service/src/completions.ts | 158 +++++++++--------- .../language-service/test/completions_spec.ts | 7 + 2 files changed, 89 insertions(+), 76 deletions(-) diff --git a/packages/language-service/src/completions.ts b/packages/language-service/src/completions.ts index f4fbdcb3f3..b4cf641aff 100644 --- a/packages/language-service/src/completions.ts +++ b/packages/language-service/src/completions.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {AbsoluteSourceSpan, AST, AstPath, AttrAst, Attribute, BoundDirectivePropertyAst, BoundElementPropertyAst, BoundEventAst, BoundTextAst, Element, ElementAst, EmptyExpr, ExpressionBinding, getHtmlTagDefinition, HtmlAstPath, Node as HtmlAst, NullTemplateVisitor, ParseSpan, ReferenceAst, TagContentType, TemplateBinding, Text, VariableBinding} from '@angular/compiler'; +import {AbsoluteSourceSpan, AST, AstPath, AttrAst, Attribute, BoundDirectivePropertyAst, BoundElementPropertyAst, BoundEventAst, BoundTextAst, Element, ElementAst, EmptyExpr, ExpressionBinding, getHtmlTagDefinition, HtmlAstPath, Node as HtmlAst, NullTemplateVisitor, ParseSpan, ReferenceAst, TagContentType, TemplateBinding, Text, VariableBinding, Visitor} from '@angular/compiler'; import {$$, $_, isAsciiLetter, isDigit} from '@angular/compiler/src/chars'; import {ATTR, getBindingDescriptor} from './binding_utils'; @@ -127,72 +127,18 @@ function getBoundedWordSpan( export function getTemplateCompletions( templateInfo: ng.AstResult, position: number): ng.CompletionEntry[] { - let result: ng.CompletionEntry[] = []; const {htmlAst, template} = templateInfo; - // The templateNode starts at the delimiter character so we add 1 to skip it. + // Calculate the position relative to the start of the template. This is needed + // because spans in HTML AST are relative. Inline template has non-zero start position. const templatePosition = position - template.span.start; - const path = getPathToNodeAtPosition(htmlAst, templatePosition); - const mostSpecific = path.tail; - if (path.empty || !mostSpecific) { - result = elementCompletions(templateInfo); - } else { - const astPosition = templatePosition - mostSpecific.sourceSpan.start.offset; - mostSpecific.visit( - { - visitElement(ast) { - const startTagSpan = spanOf(ast.sourceSpan); - const tagLen = ast.name.length; - // + 1 for the opening angle bracket - if (templatePosition <= startTagSpan.start + tagLen + 1) { - // If we are in the tag then return the element completions. - result = elementCompletions(templateInfo); - } else if (templatePosition < startTagSpan.end) { - // We are in the attribute section of the element (but not in an attribute). - // Return the attribute completions. - result = attributeCompletionsForElement(templateInfo, ast.name); - } - }, - visitAttribute(ast: Attribute) { - // An attribute consists of two parts, LHS="RHS". - // Determine if completions are requested for LHS or RHS - if (ast.valueSpan && inSpan(templatePosition, spanOf(ast.valueSpan))) { - // RHS completion - result = attributeValueCompletions(templateInfo, path); - } else { - // LHS completion - result = attributeCompletions(templateInfo, path); - } - }, - visitText(ast) { - result = interpolationCompletions(templateInfo, templatePosition); - if (result.length) return result; - const element = path.first(Element); - if (element) { - const definition = getHtmlTagDefinition(element.name); - if (definition.contentType === TagContentType.PARSABLE_DATA) { - result = voidElementAttributeCompletions(templateInfo, path); - if (!result.length) { - // If the element can hold content, show element completions. - result = elementCompletions(templateInfo); - } - } - } else { - // If no element container, implies parsable data so show elements. - result = voidElementAttributeCompletions(templateInfo, path); - if (!result.length) { - result = elementCompletions(templateInfo); - } - } - }, - visitComment() {}, - visitExpansion() {}, - visitExpansionCase() {} - }, - null); - } - + const htmlPath: HtmlAstPath = getPathToNodeAtPosition(htmlAst, templatePosition); + const mostSpecific = htmlPath.tail; + const visitor = new HtmlVisitor(templateInfo, htmlPath); + const results: ng.CompletionEntry[] = mostSpecific ? + mostSpecific.visit(visitor, null /* context */) : + elementCompletions(templateInfo); const replacementSpan = getBoundedWordSpan(templateInfo, position, mostSpecific); - return result.map(entry => { + return results.map(entry => { return { ...entry, replacementSpan, @@ -200,6 +146,78 @@ export function getTemplateCompletions( }); } +class HtmlVisitor implements Visitor { + /** + * Position relative to the start of the template. + */ + private readonly relativePosition: number; + constructor(private readonly templateInfo: ng.AstResult, private readonly htmlPath: HtmlAstPath) { + this.relativePosition = htmlPath.position; + } + // Note that every visitor method must explicitly specify return type because + // Visitor returns `any` for all methods. + visitElement(ast: Element): ng.CompletionEntry[] { + const startTagSpan = spanOf(ast.sourceSpan); + const tagLen = ast.name.length; + // + 1 for the opening angle bracket + if (this.relativePosition <= startTagSpan.start + tagLen + 1) { + // If we are in the tag then return the element completions. + return elementCompletions(this.templateInfo); + } + if (this.relativePosition < startTagSpan.end) { + // We are in the attribute section of the element (but not in an attribute). + // Return the attribute completions. + return attributeCompletionsForElement(this.templateInfo, ast.name); + } + return []; + } + visitAttribute(ast: Attribute): ng.CompletionEntry[] { + // An attribute consists of two parts, LHS="RHS". + // Determine if completions are requested for LHS or RHS + if (ast.valueSpan && inSpan(this.relativePosition, spanOf(ast.valueSpan))) { + // RHS completion + return attributeValueCompletions(this.templateInfo, this.htmlPath); + } + // LHS completion + return attributeCompletions(this.templateInfo, this.htmlPath); + } + visitText(): ng.CompletionEntry[] { + const templatePath = findTemplateAstAt(this.templateInfo.templateAst, this.relativePosition); + if (templatePath.tail instanceof BoundTextAst) { + // If we know that this is an interpolation then do not try other scenarios. + const visitor = new ExpressionVisitor( + this.templateInfo, this.relativePosition, + () => + getExpressionScope(diagnosticInfoFromTemplateInfo(this.templateInfo), templatePath)); + templatePath.tail?.visit(visitor, null); + return visitor.results; + } + // TODO(kyliau): Not sure if this check is really needed since we don't have + // any test cases for it. + const element = this.htmlPath.first(Element); + if (element && + getHtmlTagDefinition(element.name).contentType !== TagContentType.PARSABLE_DATA) { + return []; + } + // This is to account for cases like

text |

where the + // closest element has no closing tag and thus is considered plain text. + const results = voidElementAttributeCompletions(this.templateInfo, this.htmlPath); + if (results.length) { + return results; + } + return elementCompletions(this.templateInfo); + } + visitComment(): ng.CompletionEntry[] { + return []; + } + visitExpansion(): ng.CompletionEntry[] { + return []; + } + visitExpansionCase(): ng.CompletionEntry[] { + return []; + } +} + function attributeCompletions(info: ng.AstResult, path: AstPath): ng.CompletionEntry[] { const attr = path.tail; const elem = path.parentOf(attr); @@ -356,18 +374,6 @@ function elementCompletions(info: ng.AstResult): ng.CompletionEntry[] { return results; } -function interpolationCompletions(info: ng.AstResult, position: number): ng.CompletionEntry[] { - // Look for an interpolation in at the position. - const templatePath = findTemplateAstAt(info.templateAst, position); - if (!templatePath.tail) { - return []; - } - const visitor = new ExpressionVisitor( - info, position, () => getExpressionScope(diagnosticInfoFromTemplateInfo(info), templatePath)); - templatePath.tail.visit(visitor, null); - return visitor.results; -} - // There is a special case of HTML where text that contains a unclosed tag is treated as // text. For exaple '

Some ' produces a text nodes inside of the H1 // element "Some { 'trim', ]); }); + + it('should not return any results for unknown symbol', () => { + mockHost.override(TEST_TEMPLATE, '{{ doesnotexist.~{cursor} }}'); + const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor'); + const completions = ngLS.getCompletionsAtPosition(TEST_TEMPLATE, marker.start); + expect(completions).toBeUndefined(); + }); }); function expectContain( From b950d4675fb76611ee1b9af4eb35db15c46aacf0 Mon Sep 17 00:00:00 2001 From: Harri Lehtola Date: Sat, 11 Apr 2020 16:29:47 +0300 Subject: [PATCH 14/20] fix(core): do not trigger CSP alert/report in Firefox and Chrome (#36578) (#36578) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If [innerHTML] is used in a component and a Content-Security-Policy is set that does not allow inline styles then Firefox and Chrome show the following message: > Content Security Policy: The page’s settings observed the loading of a resource at self (“default-src”). A CSP report is being sent. This message is caused because Angular is creating an inline style tag to test for a browser bug that we use to decide what sanitization strategy to use, which causes CSP violation errors if inline CSS is prohibited. This test is no longer necessary, since the `DOMParser` is now safe to use and the `style` based check is redundant. In this fix, we default to using `DOMParser` if it is available and fall back to `createHTMLDocument()` if needed. This is the approach used by DOMPurify too. The related unit tests in `html_sanitizer_spec.ts`, "should not allow JavaScript execution when creating inert document" and "should not allow JavaScript hidden in badly formed HTML to get through sanitization (Firefox bug)", are left untouched to assert that the behavior hasn't changed in those scenarios. Fixes #25214. PR Close #36578 --- packages/core/src/sanitization/inert_body.ts | 68 +++----------------- 1 file changed, 9 insertions(+), 59 deletions(-) diff --git a/packages/core/src/sanitization/inert_body.ts b/packages/core/src/sanitization/inert_body.ts index 46f76eb8f4..e859f8af5a 100644 --- a/packages/core/src/sanitization/inert_body.ts +++ b/packages/core/src/sanitization/inert_body.ts @@ -9,49 +9,26 @@ /** * This helper class is used to get hold of an inert tree of DOM elements containing dirty HTML * that needs sanitizing. - * Depending upon browser support we must use one of three strategies for doing this. - * Support: Safari 10.x -> XHR strategy - * Support: Firefox -> DomParser strategy - * Default: InertDocument strategy + * Depending upon browser support we use one of two strategies for doing this. + * Default: DomParser strategy + * Fallback: InertDocument strategy */ export class InertBodyHelper { private inertDocument: Document; constructor(private defaultDoc: Document) { this.inertDocument = this.defaultDoc.implementation.createHTMLDocument('sanitization-inert'); - let inertBodyElement = this.inertDocument.body; - - if (inertBodyElement == null) { + if (this.inertDocument.body == null) { // usually there should be only one body element in the document, but IE doesn't have any, so // we need to create one. const inertHtml = this.inertDocument.createElement('html'); this.inertDocument.appendChild(inertHtml); - inertBodyElement = this.inertDocument.createElement('body'); + const inertBodyElement = this.inertDocument.createElement('body'); inertHtml.appendChild(inertBodyElement); } - inertBodyElement.innerHTML = ''; - if (inertBodyElement.querySelector && !inertBodyElement.querySelector('svg')) { - // We just hit the Safari 10.1 bug - which allows JS to run inside the SVG G element - // so use the XHR strategy. - this.getInertBodyElement = this.getInertBodyElement_XHR; - return; - } - - inertBodyElement.innerHTML = '

'; - if (inertBodyElement.querySelector && inertBodyElement.querySelector('svg img')) { - // We just hit the Firefox bug - which prevents the inner img JS from being sanitized - // so use the DOMParser strategy, if it is available. - // If the DOMParser is not available then we are not in Firefox (Server/WebWorker?) so we - // fall through to the default strategy below. - if (isDOMParserAvailable()) { - this.getInertBodyElement = this.getInertBodyElement_DOMParser; - return; - } - } - - // None of the bugs were hit so it is safe for us to use the default InertDocument strategy - this.getInertBodyElement = this.getInertBodyElement_InertDocument; + this.getInertBodyElement = isDOMParserAvailable() ? this.getInertBodyElement_DOMParser : + this.getInertBodyElement_InertDocument; } /** @@ -61,33 +38,7 @@ export class InertBodyHelper { getInertBodyElement: (html: string) => HTMLElement | null; /** - * Use XHR to create and fill an inert body element (on Safari 10.1) - * See - * https://github.com/cure53/DOMPurify/blob/a992d3a75031cb8bb032e5ea8399ba972bdf9a65/src/purify.js#L439-L449 - */ - private getInertBodyElement_XHR(html: string) { - // We add these extra elements to ensure that the rest of the content is parsed as expected - // e.g. leading whitespace is maintained and tags like `` do not get hoisted to the - // `` tag. - html = '' + html + ''; - try { - html = encodeURI(html); - } catch { - return null; - } - const xhr = new XMLHttpRequest(); - xhr.responseType = 'document'; - xhr.open('GET', 'data:text/html;charset=utf-8,' + html, false); - xhr.send(undefined); - const body: HTMLBodyElement = xhr.response.body; - body.removeChild(body.firstChild!); - return body; - } - - /** - * Use DOMParser to create and fill an inert body element (on Firefox) - * See https://github.com/cure53/DOMPurify/releases/tag/0.6.7 - * + * Use DOMParser to create and fill an inert body element in browsers that support it. */ private getInertBodyElement_DOMParser(html: string) { // We add these extra elements to ensure that the rest of the content is parsed as expected @@ -107,8 +58,7 @@ export class InertBodyHelper { /** * Use an HTML5 `template` element, if supported, or an inert body element created via * `createHtmlDocument` to create and fill an inert DOM element. - * This is the default sane strategy to use if the browser does not require one of the specialised - * strategies above. + * This is the fallback strategy if the browser does not support DOMParser. */ private getInertBodyElement_InertDocument(html: string) { // Prefer using