refactor: Adapt foundations for class-object adapters #6256

ngwattcos · 2020-07-23T19:49:21Z

…ass-object adapters for material-experimental components

This is to resolve test failures (and anticipated breaking changes) with the move to class-based adapters.

Changes:

adapter is now copied as a field (to resolve undefined adapters failing tests)
adapters are not optional and are expected to implement the full interface, because they are now class objects

...For the following components:

mdc-checkbox
mdc-chips/chip-set
mdc-chips/chip
mdc-chips/trailingaction
mdc-form-field
mdc-radio
mdc-slider
mdc-switch
mdc-tab-bar
mdc-tab-indicator

Additionally,

mdc-circular-progress
mdc-data-table
mdc-dialog
mdc-drawer/dismissible
mdc-floating-label
mdc-icon-button
mdc-line-ripple
mdc-linear-progress
mdc-list
mdc-menu-surface
mdc-menu
mdc-notched-outline
mdc-ripple
mdc-segmented-button/segment
mdc-segmented-button/segmented-button
mdc-select
mdc-select/helper-text
mdc-select/icon
mdc-snackbar

Now the base foundation constructor in the following format:

constructor(readonly adapter: MDCAdapter) {...}

The constructor for individual components have been removed, as a call to super() is sufficient to most. For the components that have initialize additional values in the constructor, the new overridden constructor is in the format:

constructor(adapter: MDCAdapter) {
  super(adapter);
  ...
}

…ass-object adapters Changes: - adapter is now copied as a field (to resolve undefined adapters failing tests) - adapters are not optional and are expected to implement the full interface, because they are now class objects Now the edited foundation constructors are in the following format: ``` constructor(readonly adapter: MDCAdapter) {...} ```

abhiomkar · 2020-07-23T20:58:13Z

Importing this PR internally.

…ter reference, removed defaultAdapter() The base class foundation now more strongly types the adapter (in anticipation of the other components moving to class-based adapters) and stores the adapter reference. Removed defaultAdapter() in the following (since we have class object adapters): mdc-checkbox mdc-chips/chip-set mdc-chips/chip mdc-chips/trailingaction mdc-form-field mdc-radio mdc-slider mdc-switch mdc-tab-bar mdc-tab-indicator

…ject adapters Deletes overridden constructors since the base foundation now expects a fully implemented adapter (except for constructors that initialize other values). Deletes all defaultAdapter() methods.

abhiomkar

Let me know when this PR is ready for re-review.

Thanks!

packages/mdc-floating-label/foundation.ts

packages/mdc-form-field/foundation.ts

packages/mdc-line-ripple/foundation.ts

packages/mdc-ripple/foundation.ts

packages/mdc-select/foundation.ts

packages/mdc-select/icon/foundation.ts

ngwattcos · 2020-07-28T16:57:11Z

@abhiomkar In case you haven't seen my message, it's ready!

abhiomkar · 2020-07-29T14:34:17Z

Re-running global tests.

abhiomkar · 2020-07-29T18:35:59Z

Commenting to Rebase internal CL.

abhiomkar · 2020-07-30T14:29:51Z

Unit tests are failing internally for most of the MDC foundation code with following error:

"MDCCheckboxFoundation defaultAdapter returns a complete adapter implementation"

Unexpected $[0] = 'addClass' in array.
Unexpected $[1] = 'forceLayout' in array.
Unexpected $[2] = 'hasNativeControl' in array.
Unexpected $[3] = 'isAttachedToDOM' in array.
Unexpected $[4] = 'isChecked' in array.
Unexpected $[5] = 'isIndeterminate' in array.
Unexpected $[6] = 'removeClass' in array.
Unexpected $[7] = 'removeNativeControlAttr' in array.
Unexpected $[8] = 'setNativeControlAttr' in array.
Unexpected $[9] = 'setNativeControlDisabled' in array.
Error: Expected $.length = 10 to equal 0.
Unexpected $[0] = 'addClass' in array.
Unexpected $[1] = 'forceLayout' in array.
Unexpected $[2] = 'hasNativeControl' in array.
Unexpected $[3] = 'isAttachedToDOM' in array.
Unexpected $[4] = 'isChecked' in array.
Unexpected $[5] = 'isIndeterminate' in array.
Unexpected $[6] = 'removeClass' in array.
Unexpected $[7] = 'removeNativeControlAttr' in array.
Unexpected $[8] = 'setNativeControlAttr' in array.
Unexpected $[9] = 'setNativeControlDisabled' in array.
    at Object.verifyDefaultAdapter

You'll need to remove defaultAdapter test case from all foundation unit tests of MDC.

Weird that unit tests are passing externally.

abhiomkar · 2020-08-03T18:52:44Z

Please let me know when this CL is ready for re-review. Thanks!

ngwattcos · 2020-08-06T22:45:25Z

@abhiomkar Should be ready! Sorry for the delay, experiencing a power outage this week

abhiomkar · 2020-08-06T23:07:18Z

Thanks Scott! No problem..

Can you resolve merge conflicts and rebase with base branch?

abhiomkar

Seems like test file updates are reverted?

abhiomkar · 2020-08-10T17:19:14Z

Hi Scott! Seems like segmented button unit tests are failing. Can you please take a look?

Thanks!

ngwattcos · 2020-08-11T20:51:36Z

@abhiomkar I've made fixes for that, but it looks like the unit tests are failing with a bunch of errors. My guess is that it's on the components side. Could you re-run the tests on your end?

abhiomkar · 2020-08-11T21:20:16Z

Verified unit tests locally. I see many unit tests are failing with following errors:

Chrome 84.0.4147 (Mac OS X 10.15.6) MDCSwitchFoundation #handleChange removes mdc-switch--checked from the switch when it is an unchecked state FAILED
	createSpyObj requires a non-empty array or object of method names to create spies for thrown

Unit tests do not run on forked repositories you'll have to manually run npm run test:unit to verify.

…ailure

ngwattcos · 2020-08-20T17:29:27Z

@abhiomkar Here's an update: the tests pass for me locally, but fail on github. I'm certain that my local repo and GitHub are running the same code.

One thing that is suspect was a single failure with announce.test.ts that I somehow suppressed by changing around the order of static fields, although it's not mentioned on GitHub.

packages/mdc-checkbox/foundation.ts

packages/mdc-chips/chip-set/test/foundation.test.ts

packages/mdc-chips/chip/foundation.ts

abhiomkar · 2020-08-20T18:44:57Z

packages/mdc-data-table/test/foundation.test.ts

 import {setUpFoundationTest} from '../../../testing/helpers/setup';
 import {attributes, cssClasses, SortValue, strings} from '../constants';
 import {MDCDataTableFoundation} from '../foundation';

 describe('MDCDataTableFoundation', () => {
-  it('default adapter returns a complete adapter implementation', () => {


is removing tests in scope of this PR? Can we remove this tests when we delete default adapters?

This may affect code coverage numbers otherwise.

@abhiomkar Initially, we removed these tests because we assumed defaultAdapter would no longer be used (since now adapters need to implement the full interface).

But another test mocks an adapter using the defaultAdapter, so just to get the tests to pass, @kseamon suggested putting back the defaultAdapter for now (we can delete them in a later PR)

@kseamon Can you please confirm if this is what you intended. http://b/165316767 suggests that unit tests will be cleaned up along with defaultAdapter code.

Sorry - missed this way back when - yes, we need to keep the default adaptors around until the tests are refactored.

The most straightforward thing might be to factor them out into objects that live in the test files.

Got it! Moving objects to live in tests SGTM so that the code coverage numbers will be intact.

Since ngwattcos@ left Google, do you want to take a stab at it? You can directly send a CL to us (Copybara will automatically merge the PR).

Let me know if you've any questions!

ngwattcos · 2020-08-20T21:11:32Z

@abhiomkar should be ready!

andrewseguin · 2022-03-29T20:08:25Z

If this is merged, we should reopen the work in angular/components#19982

googlebot added the cla: yes label Jul 23, 2020

abhiomkar added the Ready for Google label Jul 23, 2020

abhiomkar changed the title ~~refactor(material-experimental-foundations): Adapt foundations for cl…~~ refactor: Adapt foundations for class-object adapters Jul 23, 2020

ngwattcos added 2 commits July 24, 2020 15:51

refactor(components): adapt all remaining foundations to use class ob…

b226666

…ject adapters Deletes overridden constructors since the base foundation now expects a fully implemented adapter (except for constructors that initialize other values). Deletes all defaultAdapter() methods.

abhiomkar reviewed Jul 27, 2020

View reviewed changes

kseamon reviewed Jul 27, 2020

View reviewed changes

kseamon mentioned this pull request Jul 27, 2020

perf(material-experimental/mdc-snackbar): Use class for adapter angular/components#20087

Closed

ngwattcos added 2 commits July 27, 2020 15:29

refactor(mdc-components): simplify overriding constructor

ba40061

fix(chips/chip): resolve build errors

30abe58

annieyw mentioned this pull request Jul 27, 2020

feat(progress-spinner): create progress-spinner based on MDC Web angular/components#20048

Merged

abhiomkar requested review from patrickrodee and removed request for patrickrodee July 28, 2020 20:03

jelbourn mentioned this pull request Jul 29, 2020

perf(form-field): convert adapter to a class object angular/components#19982

Closed

ngwattcos requested a review from abhiomkar July 29, 2020 02:08

fix(tests): remove verifyDefaultAdapter tests for default adapters

c0ad50f

ngwattcos force-pushed the foundation-use-class-adapter branch from eb642bd to c0ad50f Compare August 6, 2020 20:36

fix(tests): remove remaining default adapter tests

89ce243

ngwattcos force-pushed the foundation-use-class-adapter branch 2 times, most recently from 553b8dc to a91c289 Compare August 7, 2020 21:41

abhiomkar requested changes Aug 7, 2020

View reviewed changes

ngwattcos force-pushed the foundation-use-class-adapter branch from a91c289 to 89ce243 Compare August 8, 2020 20:18

ngwattcos added 2 commits August 8, 2020 16:51

Merge branch 'master' into foundation-use-class-adapter

535d97d

fix(segmented-button): fix build errors

5395e39

fix(segmented-button): fix some test failures

1000147

fix(components): add back default adapters

b692a91

ngwattcos force-pushed the foundation-use-class-adapter branch from b5cc98b to b692a91 Compare August 20, 2020 02:11

ngwattcos added 2 commits August 20, 2020 12:49

lint(mdc-dom/announcer): fix lint issues that also solves unit test f…

2c7ebf2

…ailure

Merge branch 'master' into foundation-use-class-adapter

a334b06

abhiomkar requested changes Aug 20, 2020

View reviewed changes

lint(foundations): remove trailing whitespace

5758257

ngwattcos added 2 commits August 20, 2020 17:32

lint(mdc-chip): remove trailing whitespace

ca52634

fix(mdc-chips/chip-set): change property access to use string literals

2d79b01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Adapt foundations for class-object adapters #6256

refactor: Adapt foundations for class-object adapters #6256

ngwattcos commented Jul 23, 2020 •

edited

Loading

abhiomkar commented Jul 23, 2020

abhiomkar left a comment

ngwattcos commented Jul 28, 2020

abhiomkar commented Jul 29, 2020

abhiomkar commented Jul 29, 2020

abhiomkar commented Jul 30, 2020

abhiomkar commented Aug 3, 2020

ngwattcos commented Aug 6, 2020

abhiomkar commented Aug 6, 2020

abhiomkar left a comment

abhiomkar commented Aug 10, 2020

ngwattcos commented Aug 11, 2020

abhiomkar commented Aug 11, 2020

ngwattcos commented Aug 20, 2020

abhiomkar Aug 20, 2020

ngwattcos Aug 20, 2020

abhiomkar Aug 20, 2020

kseamon Jan 25, 2021

abhiomkar Jan 25, 2021

ngwattcos commented Aug 20, 2020

andrewseguin commented Mar 29, 2022

refactor: Adapt foundations for class-object adapters #6256

Are you sure you want to change the base?

refactor: Adapt foundations for class-object adapters #6256

Conversation

ngwattcos commented Jul 23, 2020 • edited Loading

abhiomkar commented Jul 23, 2020

abhiomkar left a comment

Choose a reason for hiding this comment

ngwattcos commented Jul 28, 2020

abhiomkar commented Jul 29, 2020

abhiomkar commented Jul 29, 2020

abhiomkar commented Jul 30, 2020

abhiomkar commented Aug 3, 2020

ngwattcos commented Aug 6, 2020

abhiomkar commented Aug 6, 2020

abhiomkar left a comment

Choose a reason for hiding this comment

abhiomkar commented Aug 10, 2020

ngwattcos commented Aug 11, 2020

abhiomkar commented Aug 11, 2020

ngwattcos commented Aug 20, 2020

abhiomkar Aug 20, 2020

Choose a reason for hiding this comment

ngwattcos Aug 20, 2020

Choose a reason for hiding this comment

abhiomkar Aug 20, 2020

Choose a reason for hiding this comment

kseamon Jan 25, 2021

Choose a reason for hiding this comment

abhiomkar Jan 25, 2021

Choose a reason for hiding this comment

ngwattcos commented Aug 20, 2020

andrewseguin commented Mar 29, 2022

ngwattcos commented Jul 23, 2020 •

edited

Loading