Upgrade for Angular 9

[henrahmagix]

Followed official upgrade instructions https://update.angular.io/#8.0:9.0l3 and did a bit of pruning

[alecdewitz]

Any plans to merge this soon? @willshowell

[isaackehle]

We had been trying to keep them at a base level, not to versions > x.0.0. Can you please back them down to 9.0.0 as applicable?

[isaackehle]

Also, I tried building locally w/ no success. What version of node?

[julianobrasil]

Man... I accidentally deleted my last post 😄.

Also, I tried building locally w/ no success. What version of node?

@pgkehle and @henrahmagix, after switching "module": "ESNext" to "module": "CommonJS" in tsconfig.json I could successfully build the project (after rebasing it and solving that package-lock.json conflict). You can see some explanation here: https://github.com/TypeStrong/ts-node#syntaxerror

I'm using node v13.10.1

[isaackehle]

@julianobrasil do you get any warnings/errors in the demo? Mine builds and runs now, but I get these warnings:

There is no directive with "exportAs" set to "satPopoverAnchor"
'sat-popover' is not a known element:

Also, npm run lint:

> @ncstate/sat-popover@4.0.0 lint /Users/pgkehle/code/popover-h
> ng lint popover --type-check

Linting "popover"...

WARNING: /Users/pgkehle/code/popover-h/src/lib/popover/popover-anchoring.service.ts:280:31 - keyCode is deprecated.
WARNING: /Users/pgkehle/code/popover-h/src/lib/popover/popover.component.ts:308:32 - FocusTrapFactory is deprecated: Use `ConfigurableFocusTrapFactory` instead.

[julianobrasil]

I haven't run anything yesterday, but I'll mess a little bit with it today. One thing that I've noticed by running the demo (npm run demo) is that, in anchor-reuse.component.ts, you need to change a little bit the @ViewChild() statements on these lines:

popover/src/demo/app/anchor-reuse/anchor-reuse.component.ts

Lines 39 to 40 in 35a99eb

	@ViewChild('a', { static: false }) aPopover: SatPopover;
	@ViewChild('b', { static: false }) bPopover: SatPopover;

I looks like something changed in angular 9 and you now need to run the queries before the first change detection cycle to avoid an error message on devtools console, or you will have trouble here when calling getActivePopover() before the queries have been executed internally by angular:

popover/src/demo/app/anchor-reuse/anchor-reuse.component.ts

Lines 23 to 30 in 35a99eb

	<button
	mat-button
	*ngIf="showAnchor"
	[satPopoverAnchor]="getActivePopover()"
	(click)="getActivePopover().toggle()"
	>
	Anchor
	</button>

You can solve that by just setting the static property to true on @ViewChild():

  @ViewChild('a', {static: true}) aPopover: SatPopover;
  @ViewChild('b', {static: true}) bPopover: SatPopover;

I'll dig in the other things you've pointed out.

[Pes8]

WHEN

[julianobrasil]

@pgkehle, concerning the first warning message about the keyCode in popover-anchoring.service.ts, well, it's a known issue that angular team has chosen to not change until they stop supporting IE11 (see angular/components#7374 (comment)). We can just avoid that message by disabling the lint check for that line.

Instead of:

popover/src/lib/popover/popover-anchoring.service.ts

Line 280 in 35a99eb

filter(event => event.keyCode === ESCAPE),

We can just:

// TODO: remove this line when angular stop supporting IE11 and cdk provides
//   other methods to check for the key codes in a safe way.
// tslint:disable-next-line: deprecation
filter(event => event.keyCode === ESCAPE),

[julianobrasil]

@pgkehle, the second warning is about an update of cdk a11y API. Originally there was a FocusTrapFactory.create() method, which is now deprecated in favor of ConfigurableFocusTrapFactory.create() method.

The basic difference is that the deprecated one received a boolean as its second parameter:

create(element: HTMLElement, deferCaptureElements: boolean = false): ConfigurableFocusTrap {
  ...
}

The new ConfigurableFocusTrapFactory.create, instead of a boolean, receives a ConfigurableFocusTrapConfig object:

/**
   * Creates a focus-trapped region around the given element.
   * @param element The element around which focus will be trapped.
   * @param config The focus trap configuration.
   * @returns The created focus trap instance.
   */
  create(element: HTMLElement, config?: ConfigurableFocusTrapConfig): ConfigurableFocusTrap;

As you can see, the second argument is optional, and, internally, its default value is {defer: false}.

We are not using the second parameter and, in cdk code, its default value is still false for the deferring thing:

popover/src/lib/popover/popover.component.ts

Line 398 in 35a99eb

this._focusTrap = this._focusTrapFactory.create(this._focusTrapElement.nativeElement);

By switching FocusTrapFactory to ConfigurableFocusTrapFactory, without any other change in the class, the current unit tests pass. So I think it's safe to switch classes.

[julianobrasil]

What do you thinking we should do now? Should we wait for @henrahmagix to fix all of the issues in this single PR?

Or, as an alternative, he could just fix the rebasing and the "module": "CommonJS". The warning about FocusTrapFactory would be taken care of in a tiny separate PR.

[julianobrasil]

@julianobrasil do you get any warnings/errors in the demo? Mine builds and runs now, but I get these warnings:

There is no directive with "exportAs" set to "satPopoverAnchor"
'sat-popover' is not a known element:

I think you're referring to the IDE's warnings (I didn't get this warning on the command line and I think it's just something related to @angular/language-service). They go away if you step back from using Ivy in development typescript configs (Ivy is not recommended for libraries anyway) in tsconfig.lib.ts.

"angularCompilerOptions": {
    "skipTemplateCodegen": true,
    "strictMetadataEmit": true,
    "fullTemplateTypeCheck": true,
    "strictInjectionParameters": true,
    "enableIvy": false <= added
},

npm run build (with Ivy):

npm run build (w/o Ivy):

npm run build:demo

npm run demo

npm run test:once

[isaackehle]

@julianobrasil Yes, I was referring to the IDE warnings in vs-code. If it is related just to the IDE, which it seems you just proved, then while it would be nice to find a solution for vs-code, it's not something we need to worry about.

Thanks for all of that digging. I'd agree that we should get this PR merged then do a smaller follow up for the bugs, like you suggested. While @willshowell and I were trying to keep this on the major change revision, it's just going to have to be a major release for this project.

@henrahmagix Can you please submit the change to using CommonJS today, and we'll get this merged?

[isaackehle]

Obsoleted by #185