fix(compiler-cli): use numeric comparison for TypeScript version #22705

benbraou · 2018-03-11T15:33:48Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Typescript version is compared using string compare

Issue Number: N/A
#22593

What is the new behavior?

Typescript version is compared using numeric compare

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

benbraou · 2018-03-11T15:35:35Z

packages/compiler-cli/src/transformers/program.ts

-    if (ts.version < '2.4.2' || (ts.version >= '2.7.0' && !options.disableTypeScriptVersionCheck)) {
+
+    if (compareVersions(ts.version, '2.4.2') < 0 ||
+        (compareVersions(ts.version, '2.7.0') >= 0 && !options.disableTypeScriptVersionCheck)) {
      throw new Error(


No tests were added in this PR regarding the error thrown because they are being added in #22701 .

benbraou · 2018-03-11T16:18:17Z

Bug fix separated in 2 commits:

rename symbol_query_spec.ts to typescript_symbols_spec.ts
usage of numeric comparison instead of string comparison

By the way, diff of first commit 7d45afa does show that there has been a rename (however, the full PR diff shows that a file has been deleted and a new one added)

ngbot · 2018-03-12T18:44:43Z

Hi @benbraou! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

benbraou · 2018-03-12T19:47:25Z

I am going to do the rebase to handle the recent merges
Also, to ease testing, I will isolate typescript version check a separate function

export function checkTypeScriptVersion(
  version: string,
  disableTypeScriptVersionCheck: boolean | undefined
) {
  if ((version < '2.7.2' || version >= '2.8.0') && !disableTypeScriptVersionCheck) {
    throw new Error(
        `The Angular Compiler requires TypeScript >=2.7.2 and <2.8.0 but ${version} was found instead.`);
  }
}

vicb · 2018-03-12T21:46:36Z

packages/compiler-cli/src/diagnostics/typescript_symbols.ts

@@ -858,3 +858,7 @@ function isTypescriptVersion(low: string, high?: string): boolean {
  return compareNumbers(toNumbers(low), tsNumbers) <= 0 &&
      compareNumbers(toNumbers(high), tsNumbers) >= 0;
 }
+
+export function compareVersions(version1: string, version2: string): -1|0|1 {
+  return compareNumbers(toNumbers(version1), toNumbers(version2));


isn't compareVersions('2', '2.1') === 0 with this code ?

Indeed. I did not check the implementation of compareNumbers. Fixing + adding new tests

vicb · 2018-03-12T21:48:05Z

The first commit should be a refactor (does not "fix" anything). Thanks.

benbraou · 2018-03-13T04:33:41Z

@vicb Thanks for the review. PR updated

Utility functions that check TypeScript version are moved to a new file typescript_version.ts
compareNumbers method is updated
unit tests added for typescript_version.ts
unit test added for checkTypeScriptVersion in program_spec.ts
Only one commit is now needed. symbol_query_spec.ts is no longer renamed

vicb · 2018-03-22T18:51:28Z

packages/compiler-cli/src/transformers/program.ts

@@ -751,6 +748,14 @@ class AngularCompilerProgram implements Program {
  }
 }

+export function checkTypeScriptVersion(
+    version: string, disableTypeScriptVersionCheck: boolean | undefined) {
+  if ((version < '2.7.2' || version >= '2.8.0') && !disableTypeScriptVersionCheck) {


We should probably use compareVersions here ?

Also what about moving the version strings to const MIN_TS_VERSION and MAX_TS_VERSION ?

Indeed. I will implemented the change for both comments

vicb · 2018-03-22T18:52:12Z

packages/compiler-cli/test/diagnostics/typescript_version_spec.ts

+
+describe('toNumbers', () => {
+
+  it('should handle undefined value', () => { expect(toNumbers(undefined)).toEqual([]); });


should it ?

I was viewing this test as a "behavior documentation" of a method implemented prior to my commit.

No, it should not handle undefined. I am changing the signature and implementation of toNumbers

Great catch btw. This allowed me to discover a regression that would have been introduced. Please check my comment in describe('isTypescriptVersion', () => {

vicb · 2018-03-22T18:52:42Z

packages/compiler-cli/test/diagnostics/typescript_version_spec.ts

+  it('should handle undefined value', () => { expect(toNumbers(undefined)).toEqual([]); });
+
+  it('should handle strings', () => {
+    expect(toNumbers('')).toEqual([]);


Why would this ever be valid ?

I can't think of a reason. I am removing this assertion but keeping the method implementation. Please let me know in case this is not enough

vicb

Mostly LGTM.

One change required,
Why do we need to support toNumbers returning an empty array ?

benbraou · 2018-03-23T20:30:03Z

packages/compiler-cli/test/diagnostics/typescript_version_spec.ts

+
+describe('toNumbers', () => {
+
+  it('should handle undefined value', () => { expect(toNumbers(undefined)).toEqual([]); });


I was viewing this test as a "behavior documentation" of a method implemented prior to my commit.

No, it should not handle undefined. I am changing the signature and implementation of toNumbers

Great catch btw. This allowed me to discover a regression that would have been introduced. Please check my comment in describe('isTypescriptVersion', () => {

benbraou · 2018-03-23T20:30:09Z

packages/compiler-cli/test/diagnostics/typescript_version_spec.ts

+  it('should handle undefined value', () => { expect(toNumbers(undefined)).toEqual([]); });
+
+  it('should handle strings', () => {
+    expect(toNumbers('')).toEqual([]);


I can't think of a reason. I am removing this assertion but keeping the method implementation. Please let me know in case this is not enough

benbraou · 2018-03-23T20:30:33Z

packages/compiler-cli/test/diagnostics/typescript_version_spec.ts

+describe('isTypescriptVersion', () => {
+  it('should correctly check if a typescript version is within a given range', () => {
+    expect(isTypeScriptVersion('2.7.0', '2.40')).toEqual(false);
+    expect(isTypeScriptVersion('2.40', '2.7.0')).toEqual(false);


This assertion is incorrect.
2.40 > 2.7.0 => isTypeScriptVersion('2.40', '2.7.0') should be true and not false
Why is this happening ?

export function isTypeScriptVersion(version: string, low: string, high?: string): boolean { const tsNumbers = toNumbers(version); return compareNumbers(toNumbers(low), tsNumbers) <= 0 && compareNumbers(toNumbers(high), tsNumbers) >= 0; }

isTypeScriptVersion('2.40', '2.7.0') ( => high = undefined) uses internally the comparison compareNumbers(toNumbers(undefined),[2, 40]) >= 0

toNumbers(undefined) is returning [] (This is counter intuitive as @vicb noticed)

So we end up comparing [] and [2,40]

Prior to commit, [] = [2,40] because the comparison stops at the min length of both array (here 0) and defaulting to 0

With my commit, [] < [2,40] (and this makes a lot of sense for me). So, compareNumbers(toNumbers(undefined),[2, 40] ) >= 0 returns false . And so,isTypeScriptVersion('2.40', '2.7.0') incorrectly returns false

Fix ?

as @vicb suggested, toNumbers(undefined) should not even compile

Improve the implementation of isTypeScriptVersion: check high parameter is defined before performing the comparison

isTypeScriptVersion is only used in typescript_symbols.ts to create SymbolTable. Since this regression did not break any test, I am adding a unit test for toSymbolTable method

benbraou · 2018-03-23T22:08:42Z

@vicb thanks for the review. All requested changes/questions are addressed and explained in my comments.
A regression with isTypeScriptVersion was fixed (I have explained my finding in my review of typescript_version_spec.ts)
I have added two new commits to better highlight the changes.

I see that e2e tests are failing because of this error
Error: XHR error loading http://localhost:8000/all/benchmarks/vendor/incremental-dom-cjs.js
I don't see how it is linked to this PR but I am having a look (maybe trigger a new build ?)

benbraou · 2018-03-24T16:21:21Z

After forcing a new build, I only see CI_MODE=saucelabs_required failing due to timeout issues. I am seeing this same failure in other ongoing PRs

vicb · 2018-03-25T01:08:23Z

packages/compiler-cli/src/diagnostics/typescript_symbols.ts


+const MIN_TS_VERSION_SUPPORTING_MAP = '2.2';


move that right before where it is used and/or add a one line comment

will be moved and commented

vicb · 2018-03-25T01:09:10Z

packages/compiler-cli/src/diagnostics/typescript_symbols.ts

-      }
-      return result as ts.SymbolTable;
-    });
+export const toSymbolTable = (tsVersion: string) => (symbols: ts.Symbol[]) => {


Rename toSymbolTableFactory ?

vicb · 2018-03-29T02:48:32Z

http://test/OCL:190874633:BASE:190874643:1522291696988:7171a71f

vicb · 2018-03-29T05:24:02Z

blocked on failing tests internally

[...]/packages/compiler-cli/src/diagnostics/typescript_symbols.ts(410,12): error TS2352: Type 'Map<string, Symbol>' cannot be converted to type 'UnderscoreEscapedMap<Symbol>'.
  Types of property 'forEach' are incompatible.
    Type '(callbackfn: (value: Symbol, key: string, map: Map<string, Symbol>) => void, thisArg?: any) => void' is not comparable to type '(action: (value: Symbol, key: __String) => void) => void'.
      Types of parameters 'callbackfn' and 'action' are incompatible.
        Types of parameters 'key' and 'key' are incompatible.
          Type 'string' is not comparable to type '__String'.

benbraou · 2018-03-29T16:51:46Z

@vicb in typescript_symbols.ts, I am going to add any type assertion

So what we currently have:

  // ∀ Typescript version >= 2.2, `SymbolTable` is implemented as an ES6 `Map`
    const result = new Map<string, ts.Symbol>();
    for (const symbol of symbols) {
      result.set(symbol.name, symbol);
    }
    return <ts.SymbolTable>result;

will become

export const toSymbolTableFactory = (tsVersion: string) => (symbols: ts.Symbol[]) => {
  if (isVersionBetween(tsVersion, MIN_TS_VERSION_SUPPORTING_MAP)) {
    // ∀ Typescript version >= 2.2, `SymbolTable` is implemented as an ES6 `Map`
    const result = new Map<string, ts.Symbol>();
    for (const symbol of symbols) {
      result.set(symbol.name, symbol);
    }
    // First, tell the compiler that `result` is of type `any`. Then, use a second type assertion
    // to `ts.SymbolTable`.
    // Otherwise, `Map<string, ts.Symbol>` and `ts.SymbolTable` will be considered as incompatible
    // types by the compiler
    return <ts.SymbolTable>(<any>result);
  }

I am going to amend 6f02545

Fixes angular#22593

benbraou · 2018-03-29T17:40:44Z

rebased on top of master again to force a new build.
Previous one has a failing job CI_MODE=saucelabs_required

Fixes angular#22593

benbraou · 2018-03-29T18:21:25Z

forced a new build due to failure now in aio_e2e build job

vicb · 2018-03-29T19:04:48Z

http://test/OCL:190874633:BASE:190965443:1522350276162:99bb7658

alexeagle

bazel change looks fine to me

vicb · 2018-03-29T21:48:35Z

@benbraou This is ready to be merged. Thank you very much for your work on that.

vicb · 2018-03-29T21:49:35Z

merge-assistance: This PR is green - ci/angular status is off

Fixes #22593 PR Close #22705

angular-automatic-lock-bot · 2019-09-13T11:51:37Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Mar 11, 2018

benbraou commented Mar 11, 2018

View reviewed changes

benbraou force-pushed the issue22593 branch 2 times, most recently from 4e23a0b to c21f67f Compare March 11, 2018 16:12

kara added area: core Issues related to the framework runtime action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 12, 2018

benbraou changed the title ~~fix(compiler-cli): use numeric comparison for TypeScript version~~ [WIP] fix(compiler-cli): use numeric comparison for TypeScript version Mar 12, 2018

vicb reviewed Mar 12, 2018

View reviewed changes

benbraou force-pushed the issue22593 branch 2 times, most recently from b9412eb to 51bb8b6 Compare March 13, 2018 03:43

benbraou changed the title ~~[WIP] fix(compiler-cli): use numeric comparison for TypeScript version~~ fix(compiler-cli): use numeric comparison for TypeScript version Mar 13, 2018

benbraou mentioned this pull request Mar 13, 2018

test(compiler-cli): add tests for supported TS versions #22701

Closed

3 tasks

vicb reviewed Mar 22, 2018

View reviewed changes

vicb suggested changes Mar 22, 2018

View reviewed changes

benbraou commented Mar 23, 2018

View reviewed changes

benbraou force-pushed the issue22593 branch from 51bb8b6 to e3cc3a6 Compare March 23, 2018 20:56

benbraou force-pushed the issue22593 branch 2 times, most recently from ea67dc6 to 387826e Compare March 24, 2018 15:21

benbraou force-pushed the issue22593 branch from 387826e to fec2264 Compare March 24, 2018 19:55

vicb reviewed Mar 25, 2018

View reviewed changes

vicb removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Mar 29, 2018

vicb self-assigned this Mar 29, 2018

vicb added the state: blocked label Mar 29, 2018

benbraou force-pushed the issue22593 branch from 8a69565 to 85cf698 Compare March 29, 2018 16:52

fix(compiler-cli): use numeric comparison for TypeScript version

00ebb33

Fixes angular#22593

benbraou force-pushed the issue22593 branch from 85cf698 to 1b5a3ec Compare March 29, 2018 17:39

build(compiler-cli): include new test files in bazel config

afd8ed3

Fixes angular#22593

benbraou force-pushed the issue22593 branch from 1b5a3ec to afd8ed3 Compare March 29, 2018 18:20

alexeagle approved these changes Mar 29, 2018

View reviewed changes

vicb added action: merge The PR is ready for merge by the caretaker and removed state: blocked labels Mar 29, 2018

vicb added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Mar 29, 2018

ocombe added target: major This PR is targeted for the next major release and removed merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release labels Mar 30, 2018

alxhub closed this in 193737a Mar 30, 2018

alxhub pushed a commit that referenced this pull request Mar 30, 2018

build(compiler-cli): include new test files in bazel config (#22705)

a5f0939

Fixes #22593 PR Close #22705

benbraou deleted the issue22593 branch March 30, 2018 17:20

benbraou mentioned this pull request Mar 30, 2018

no tests for disableTypeScriptVersionCheck angularCompilerOptions flag #22699

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019


		describe('toNumbers', () => {

		it('should handle undefined value', () => { expect(toNumbers(undefined)).toEqual([]); });

fix(compiler-cli): use numeric comparison for TypeScript version #22705

fix(compiler-cli): use numeric comparison for TypeScript version #22705

Conversation

benbraou commented Mar 11, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Choose a reason for hiding this comment

benbraou commented Mar 11, 2018

ngbot bot commented Mar 12, 2018

benbraou commented Mar 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb commented Mar 12, 2018

benbraou commented Mar 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benbraou commented Mar 23, 2018

benbraou commented Mar 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb commented Mar 29, 2018

vicb commented Mar 29, 2018 • edited

benbraou commented Mar 29, 2018

benbraou commented Mar 29, 2018

benbraou commented Mar 29, 2018

vicb commented Mar 29, 2018

alexeagle left a comment

Choose a reason for hiding this comment

vicb commented Mar 29, 2018

vicb commented Mar 29, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

vicb commented Mar 29, 2018 •

edited