feat: upgrade to angular 13 #1996

pavankjadda · 2021-11-05T20:20:00Z

No description provided.

… support those

pavankjadda · 2021-11-06T01:29:12Z

@varnastadeus when you get a chance please review this PR and approve it

inexuscore · 2021-11-07T20:09:43Z

Any idea when Angular 13 support will be available? Kinda desperately need to update to Angular 13 ASAP. Thank you.

pavankjadda · 2021-11-09T01:39:23Z

Any idea when Angular 13 support will be available? Kinda desperately need to update to Angular 13 ASAP. Thank you.

Hopefully soon. @varnastadeus reviewing the code now.

inexuscore · 2021-11-09T07:58:46Z

@pavankjadda That's awesome, thank you!

terencehonles · 2021-11-09T11:51:56Z

angular.json

@@ -31,16 +34,6 @@
              "src/ng-select/lib/ng-templates.directive.ts"
            ]
          }
-        },
-        "lint": {
-          "builder": "@angular-devkit/build-angular:tslint",


Should eslint not be enabled instead of just dropping tslint? Specifically builder: @angular-eslint/builder:lint

This looks like why the CI is failing too since there is now no lint command.

@pavankjadda can you address this?

Since angular removed tslint, I think we should add @angular-eslint to the project and remove tslint. And angular-eslint still adding the support. Perhaps we can add beta version of the library.

okay. I added angular-eslint to the project

terencehonles

@pavankjadda thanks for adding the lint config back, a few questions on it 🙂

terencehonles · 2021-11-09T19:12:07Z

src/ng-option-highlight/.eslintrc.json

+        "@typescript-eslint/dot-notation": "off",
+        "@typescript-eslint/explicit-member-accessibility": [
+          "off",
+          {
+            "accessibility": "explicit"
+          }
+        ],
+        "@typescript-eslint/member-delimiter-style": [
+          "off",
+          {
+            "multiline": {
+              "delimiter": "none",
+              "requireLast": true
+            },
+            "singleline": {
+              "delimiter": "semi",
+              "requireLast": false
+            }
+          }
+        ],
+        "@typescript-eslint/semi": [
+          "off",
+          null
+        ],
+        "brace-style": [
+          "error",
+          "1tbs"
+        ],
+        "id-blacklist": "off",
+        "id-match": "off",
+        "no-redeclare": "error",
+        "no-trailing-spaces": "off",
+        "no-underscore-dangle": "off"


Should we fix these so we don't have to turn these off? I assume they must be triggering errors if they're not matching what was in the previous tslist.json

@angular-eslint library converted all rules from tslint.json to this file. But to be honest I haven't looked at the file closely, since they do good job of it. But I am not sure if we need to tackle this in this PR or have separate for it.

terencehonles · 2021-11-09T19:50:42Z

src/ng-select/.eslintrc.json

+      "rules": {
+        "@angular-eslint/directive-selector": [
+          "error",
+          {
+            "type": "attribute",
+            "prefix": "lib",
+            "style": "camelCase"
+          }
+        ],
+        "@angular-eslint/component-selector": [
+          "error",
+          {
+            "type": "element",
+            "prefix": "lib",
+            "style": "kebab-case"
+          }
+        ]
+      }


do you need to specify these since they are defined in .eslintrc.json and it looks like this should extend that config? (The same applies to src/ng-option-highlight/.eslintrc.json)

terencehonles · 2021-11-09T19:54:59Z

.eslintrc.json

+        "createDefaultProgram": true
+      },
+      "extends": [
+        "plugin:@angular-eslint/recommended",


Does recommended cover all the rules in tslint.json?

I ran lint based on this config, I got following report

/Users/jaddap2/WebstormProjects/ng-select/src/ng-select/lib/ng-dropdown-panel.component.ts 35:15 error The selector should start with one of these prefixes: "lib" (https://angular.io/guide/styleguide#style-02-07) @angular-eslint/component-selector /Users/jaddap2/WebstormProjects/ng-select/src/ng-select/lib/ng-option.component.ts 14:15 error The selector should start with one of these prefixes: "lib" (https://angular.io/guide/styleguide#style-02-07) @angular-eslint/component-selector /Users/jaddap2/WebstormProjects/ng-select/src/ng-select/lib/ng-select.component.spec.ts 4150:7 error Component class names should end with one of these suffixes: "Component" (https://angular.io/styleguide#style-02-03) @angular-eslint/component-class-suffix 4241:7 error Component class names should end with one of these suffixes: "Component" (https://angular.io/styleguide#style-02-03) @angular-eslint/component-class-suffix 4248:7 error Component class names should end with one of these suffixes: "Component" (https://angular.io/styleguide#style-02-03) @angular-eslint/component-class-suffix /Users/jaddap2/WebstormProjects/ng-select/src/ng-select/lib/ng-select.component.ts 62:15 error The selector should start with one of these prefixes: "lib" (https://angular.io/guide/styleguide#style-02-07) @angular-eslint/component-selector 72:5 error Use @HostBinding or @HostListener rather than the `host` metadata property (https://angular.io/styleguide#style-06-03) @angular-eslint/no-host-metadata-property 258:5 warning Lifecycle interface 'OnInit' should be implemented for method 'ngOnInit'. (https://angular.io/styleguide#style-09-01) @angular-eslint/use-lifecycle-interface /Users/jaddap2/WebstormProjects/ng-select/src/ng-select/lib/ng-templates.directive.ts 4:24 error The selector should start with one of these prefixes: "lib" (https://angular.io/guide/styleguide#style-02-08) @angular-eslint/directive-selector 18:24 error The selector should be camelCase (https://angular.io/guide/styleguide#style-02-06) @angular-eslint/directive-selector 23:24 error The selector should be camelCase (https://angular.io/guide/styleguide#style-02-06) @angular-eslint/directive-selector 28:24 error The selector should be camelCase (https://angular.io/guide/styleguide#style-02-06) @angular-eslint/directive-selector 33:24 error The selector should be camelCase (https://angular.io/guide/styleguide#style-02-06) @angular-eslint/directive-selector 38:24 error The selector should be camelCase (https://angular.io/guide/styleguide#style-02-06) @angular-eslint/directive-selector 43:24 error The selector should be camelCase (https://angular.io/guide/styleguide#style-02-06) @angular-eslint/directive-selector 48:24 error The selector should be camelCase (https://angular.io/guide/styleguide#style-02-06) @angular-eslint/directive-selector 53:24 error The selector should be camelCase (https://angular.io/guide/styleguide#style-02-06) @angular-eslint/directive-selector 58:24 error The selector should be camelCase (https://angular.io/guide/styleguide#style-02-06) @angular-eslint/directive-selector 63:24 error The selector should be camelCase (https://angular.io/guide/styleguide#style-02-06) @angular-eslint/directive-selector 68:24 error The selector should be camelCase (https://angular.io/guide/styleguide#style-02-06) @angular-eslint/directive-selector

we can the following errors since they are false positive

The selector should be camelCase

The selector should start with one of these prefixes: "lib"

but we may want to resolve this

72:5 error Use @HostBinding or @HostListener rather than the `host` metadata property (https://angular.io/styleguide#style-06-03) @angular-eslint/no-host-metadata-property

Fixed on https://github.com/pavankjadda/ng-select/pull/1 (first commit)

terencehonles · 2021-11-11T12:59:25Z

@pavankjadda I created https://github.com/pavankjadda/ng-select/pull/1 against your branch that this PR is created off of including my suggestions and additional eslint config changes. You can merge the PR to update this PR with my suggestions.

…-add-on rules

suggestions to #1996

pavankjadda · 2021-11-11T18:19:36Z

@terencehonles merged your changes. All files passing lint rules. This is good to go now.

pavankjadda · 2021-11-11T18:21:22Z

@varnastadeus please review this

terencehonles · 2021-11-11T21:18:39Z

@terencehonles merged your changes. All files passing lint rules. This is good to go now.

Thanks for the follow up @pavankjadda. @varnastadeus I followed up to confirm the lint rules match the original intent (some things slipped through because tslint didn't catch everything it was configured to)

terencehonles

Finishing a full review, LGTM!

terencehonles · 2021-11-11T21:21:59Z

angular.json

@@ -1,5 +1,9 @@
 {
  "$schema": "./node_modules/@angular/cli/lib/config/schema.json",
+  "cli": {


This is probably fine, but it probably can also be dropped. Not sure if you have a preference @varnastadeus

pavankjadda · 2021-11-11T22:07:05Z

Finishing a full review, LGTM!

Do you have access to merge this and release new version?

terencehonles · 2021-11-11T22:31:12Z

Finishing a full review, LGTM!

Do you have access to merge this and release new version?

Nope, that would be @varnastadeus, just letting them know I thought the PR looked good 🙂

pavankjadda · 2021-11-15T13:36:58Z

@varnastadeus When you get a chance please review and approve this.

varnastadeus · 2021-11-15T14:51:15Z

@varnastadeus When you get a chance please review and approve this.

@pavankjadda tests are not running in the CI job. Can you check on this?

pavankjadda · 2021-11-15T14:52:57Z

@varnastadeus When you get a chance please review and approve this.

@pavankjadda tests are not running in the CI job. Can you check on this?

I am able to run them on local machine. It seems some kind of issue with GitHub actions when using Karma

varnastadeus · 2021-11-15T15:01:20Z

@varnastadeus When you get a chance please review and approve this.

@pavankjadda tests are not running in the CI job. Can you check on this?

I am able to run them on local machine. It seems some kind of issue with GitHub actions when using Karma

@pavankjadda I am having the same error while running yarn test locally.

pavankjadda · 2021-11-15T15:13:26Z

@varnastadeus When you get a chance please review and approve this.

@pavankjadda tests are not running in the CI job. Can you check on this?

I am able to run them on local machine. It seems some kind of issue with GitHub actions when using Karma

@pavankjadda I am having the same error while running yarn test locally.

Interesting. Let me check

pavankjadda · 2021-11-15T15:44:37Z

@varnastadeus would you mind enabling CI workflow on each commit and pull request. I just pushed an update and I don't know if it passes CI

pavankjadda · 2021-11-15T15:51:38Z

I see all tests passing (272/273) expect this one

            it('should allow edit search if option selected & input focused', fakeAsync(() => {
                const fixture = createTestingModule(
                    NgSelectTestComponent,
                    `<ng-select [items]="cities"
                        [typeahead]="filter"
                        [editableSearchTerm]="true"
                        bindValue="id"
                        bindLabel="name"
                        [(ngModel)]="selectedCity">
                    </ng-select>`);
                expect(fixture.componentInstance.select.editableSearchTerm).toBeTruthy();
                const select = fixture.componentInstance.select;
                const input = select.searchInput.nativeElement;
                const selectedCity = fixture.componentInstance.cities[0];
                fixture.componentInstance.selectedCity = selectedCity.id;
                tickAndDetectChanges(fixture);
                input.focus();
                tickAndDetectChanges(fixture);
                expect(select.searchTerm).toEqual(selectedCity.name);
                expect(input.value).toEqual(selectedCity.name);
            }));

I see the following error message in Console. I wonder if this was introduced by changes from @terencehonles. But that's highly unlikely

	Error: Expected null to equal 'Vilnius'.
	    at <Jasmine>
	    at UserContext.<anonymous> (src/ng-select/lib/ng-select.component.spec.ts:3054:43)
	    at UserContext.fakeAsyncFn (node_modules/zone.js/fesm2015/zone-testing.js:1968:1)
	    at ZoneDelegate.invoke (node_modules/zone.js/fesm2015/zone.js:372:1)
	Error: Expected '' to equal 'Vilnius'.
	    at <Jasmine>
	    at UserContext.<anonymous> (src/ng-select/lib/ng-select.component.spec.ts:3055:37)
	    at UserContext.fakeAsyncFn (node_modules/zone.js/fesm2015/zone-testing.js:1968:1)
	    at ZoneDelegate.invoke (node_modules/zone.js/fesm2015/zone.js:372:1)

src/ng-select/lib/ng-select.component.spec.ts

terencehonles

I believe you can probably remove from the karma config and the package.json karma-coverage-istanbul-reporter

src/ng-option-highlight/karma.conf.js

src/ng-select/karma.conf.js

pavankjadda · 2021-11-15T17:20:35Z

@varnastadeus all the tests are passing on my local machine. You can review this now.

package.json

…from karma config. 1. removes unsupported `karma-coverage-istanbul-reporter` plugin from karma config. 2. see https://github.com/angular/angular-cli/blob/master/CHANGELOG.md#angular-devkitbuild-angular-3 for more details on this

varnastadeus · 2021-11-16T09:02:43Z

diff --git a/src/ng-select/karma.conf.js b/src/ng-select/karma.conf.js
--- src/ng-select/karma.conf.js
+++ src/ng-select/karma.conf.js
@@ -14,11 +14,11 @@
         ],
         client: {
             clearContext: false // leave Jasmine Spec Runner output visible in browser
         },
-        coverageIstanbulReporter: {
+        coverageReporter: {
             dir: require('path').join(__dirname, '../../coverage/ng-select'),
-            reports: ['lcovonly'],
+            reporters: ['lcovonly'],
             fixWebpackSourcePaths: true
         },
         reporters: ['progress', 'kjhtml'],
         port: 9876,

terencehonles · 2021-11-16T09:21:18Z

@varnastadeus I was about to do the same 😅 it looks like we may want to also pull in angular/angular-cli@ef8815d but when updating to Angular 13.0.2 I noticed that I had to use --force since @ng-bootstrap/ng-bootstrap isn't already updated for Angular 13 ng-bootstrap/ng-bootstrap#4178 we may want to wait for that before merging this (or at least packaging this).

terencehonles · 2021-11-16T09:33:55Z

These are the changes I made https://github.com/pavankjadda/ng-select/pull/2

terencehonles · 2021-11-16T09:36:54Z

Since you have push access to @pavankjadda's branch you can fast forward it with git fetch git@github.com:terencehonles/ng-select.git update-karma-angular-and-ng-bootstrap && git push git@github.com:pavankjadda/ng-select.git FETCH_HEAD:angular13-support I can't do that however since I'm not a maintainer of this repo and don't inherit that permission.

pavankjadda · 2021-11-16T14:09:18Z

diff --git a/src/ng-select/karma.conf.js b/src/ng-select/karma.conf.js
--- src/ng-select/karma.conf.js
+++ src/ng-select/karma.conf.js
@@ -14,11 +14,11 @@
         ],
         client: {
             clearContext: false // leave Jasmine Spec Runner output visible in browser
         },
-        coverageIstanbulReporter: {
+        coverageReporter: {
             dir: require('path').join(__dirname, '../../coverage/ng-select'),
-            reports: ['lcovonly'],
+            reporters: ['lcovonly'],
             fixWebpackSourcePaths: true
         },
         reporters: ['progress', 'kjhtml'],
         port: 9876,

@varnastadeus Thanks for this. I still see CI failing in Coverall stage. Any idea on how to fix it?

Update: Never mind I still old Karma config in one file. Updated it, CI should pass now

pavankjadda · 2021-11-16T15:33:15Z

@varnastadeus never mind. I updated karma config based on latest configuration changes. I tested them in another branch in my fork. All works fine and CI is passing. You can merge the PR now

varnastadeus · 2021-11-16T16:11:22Z

thanks again for the PR guys 🎉

coveralls · 2021-11-16T16:19:29Z

Coverage increased (+0.04%) to 93.604% when pulling 2e40d2a on pavankjadda:angular13-support into 37b589a on ng-select:master.

pavankjadda · 2021-11-16T16:36:01Z

thanks again for the PR guys 🎉

No problem. Happy to contribute. I see GitHub actions failed to release new version. You can see the build log here

varnastadeus · 2021-11-16T16:50:39Z

@pavankjadda I have addressed those issues here #2004

github-actions · 2021-11-16T17:11:49Z

🎉 This PR is included in version 8.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

pavankjadda added 2 commits November 5, 2021 16:19

feat: upgrade to angular 13

dd05ea3

chore: changes node minimum version to 12.20.0 as angular 13 does not…

e093687

… support those

docs: updates version matrix on README.md

ed8cfe2

varnastadeus self-requested a review November 8, 2021 12:26

terencehonles reviewed Nov 9, 2021

View reviewed changes

feat(lint): adds eslint to the project

e3936ec

terencehonles reviewed Nov 9, 2021

View reviewed changes

terencehonles added 2 commits November 11, 2021 13:53

update eslint config and fix rule violations

7660996

update and fix linting with @angular-eslint/ng-cli-compat rules

3b5d665

terencehonles and others added 4 commits November 11, 2021 14:10

update and fix linting with @angular-eslint/ng-cli-compat--formatting…

15d0509

…-add-on rules

update eslint to better match the previous tslint.json

85482c5

Merge pull request #1 from terencehonles/angular13-support

042f6e1

suggestions to #1996

chore(lint): fixes indent issues reported by lint

212bfef

terencehonles approved these changes Nov 11, 2021

View reviewed changes

Platonn mentioned this pull request Nov 15, 2021

Upgrade to Angular 13 SAP/spartacus#13916

Closed

69 tasks

tests: adds missing karma plugin karma-coverage

80817a0

terencehonles reviewed Nov 15, 2021

View reviewed changes

src/ng-select/lib/ng-select.component.spec.ts Show resolved Hide resolved

terencehonles reviewed Nov 15, 2021

View reviewed changes

src/ng-option-highlight/karma.conf.js Outdated Show resolved Hide resolved

src/ng-select/karma.conf.js Outdated Show resolved Hide resolved

tests: adds asynchronous wait for one of the tests to resolve

81e59b6

terencehonles reviewed Nov 15, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

Update karma.conf.js

2cae954

varnastadeus approved these changes Nov 16, 2021

View reviewed changes

pavankjadda requested a review from varnastadeus November 16, 2021 14:08

chore(karma): updates karma.conf.js with new coverageReporter config

2e40d2a

varnastadeus approved these changes Nov 16, 2021

View reviewed changes

varnastadeus merged commit 45b61c7 into ng-select:master Nov 16, 2021

pavankjadda deleted the angular13-support branch November 16, 2021 16:28

github-actions bot added the released label Nov 16, 2021

varnastadeus mentioned this pull request Nov 16, 2021

Angular 13 support #1992

Closed

terencehonles mentioned this pull request Nov 16, 2021

fix: fix scss files not being exported for Angular 13 #2007

Merged

feat: upgrade to angular 13 #1996

feat: upgrade to angular 13 #1996

Conversation

pavankjadda commented Nov 5, 2021

pavankjadda commented Nov 6, 2021 • edited

inexuscore commented Nov 7, 2021

pavankjadda commented Nov 9, 2021

inexuscore commented Nov 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavankjadda Nov 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencehonles 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencehonles Nov 11, 2021 • edited

Choose a reason for hiding this comment

terencehonles commented Nov 11, 2021

pavankjadda commented Nov 11, 2021

pavankjadda commented Nov 11, 2021

terencehonles commented Nov 11, 2021

terencehonles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavankjadda commented Nov 11, 2021

terencehonles commented Nov 11, 2021

pavankjadda commented Nov 15, 2021

varnastadeus commented Nov 15, 2021

pavankjadda commented Nov 15, 2021

varnastadeus commented Nov 15, 2021

pavankjadda commented Nov 15, 2021

pavankjadda commented Nov 15, 2021

pavankjadda commented Nov 15, 2021 • edited

terencehonles left a comment

Choose a reason for hiding this comment

pavankjadda commented Nov 15, 2021 • edited

varnastadeus commented Nov 16, 2021

terencehonles commented Nov 16, 2021

terencehonles commented Nov 16, 2021

terencehonles commented Nov 16, 2021

pavankjadda commented Nov 16, 2021 • edited

pavankjadda commented Nov 16, 2021

varnastadeus commented Nov 16, 2021

coveralls commented Nov 16, 2021

pavankjadda commented Nov 16, 2021

varnastadeus commented Nov 16, 2021

github-actions bot commented Nov 16, 2021

pavankjadda commented Nov 6, 2021 •

edited

pavankjadda Nov 9, 2021 •

edited

terencehonles Nov 11, 2021 •

edited

pavankjadda commented Nov 15, 2021 •

edited

pavankjadda commented Nov 15, 2021 •

edited

pavankjadda commented Nov 16, 2021 •

edited