Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug(CdkTable): CdkTable constructor parameters changed in a patch version #20422

Closed
szymonbultrowicz opened this issue Aug 26, 2020 · 4 comments · Fixed by #20425
Closed

bug(CdkTable): CdkTable constructor parameters changed in a patch version #20422

szymonbultrowicz opened this issue Aug 26, 2020 · 4 comments · Fixed by #20425
Assignees
Labels
area: cdk/table P2 The issue is important to a large percentage of users, with a workaround regression This issue is related to a regression

Comments

@szymonbultrowicz
Copy link

The constructor parameters of CdkTable changed in version 10.1.2 (see: 10.1.1...10.1.2#diff-86c33ad9a24e48674baaf250f4035a11).

This issue might be not considered a bug if we do not assume CDK components API is something that should conform semver versioning.
However, in my opinion, parameters of a public constructor should be rather treated as a part of the public API. Especially for a CDK, which is meant to be used and extended by others. Thus such a breaking change should be done in major versions only according to the semver standard.

The particular breaking change:
71a2a17#diff-86c33ad9a24e48674baaf250f4035a11R430

The change causes our Barista library to fail the compilation. However, I've originally spotted the problem during development of our product that uses the Barista lib and it was much more cumbersome, because there was no compilation error - the library was already prebuilt and the only error that we got was a runtime one (both Jest and browser):

    TypeError: Cannot read property 'nativeElement' of null

      at DtTable.CdkTable (../src/cdk/table/table.ts:442:24)
      at DtTable._DtTableBase [as constructor] (libs/barista-components/table/src/base-table.ts:64:5)
      at new DtTable (libs/barista-components/table/src/table.ts:166:5)
      at createClass (../packages/core/src/view/provider.ts:290:7)
      at createDirectiveInstance (../packages/core/src/view/provider.ts:142:7)
      at createViewNodes (../packages/core/src/view/view.ts:314:28)
      at callViewAction (../packages/core/src/view/view.ts:647:7)
      at execComponentViewsAction (../packages/core/src/view/view.ts:570:7)
      at createViewNodes (../packages/core/src/view/view.ts:342:3)
      at createRootView (../packages/core/src/view/view.ts:216:3)
      at callWithDebugContext (../packages/core/src/view/services.ts:638:23)
      at Object.debugCreateRootView [as createRootView] (../packages/core/src/view/services.ts:119:10)
      at ComponentFactory_.create (../packages/core/src/view/refs.ts:92:27)
      at initComponent (../packages/core/testing/src/test_bed.ts:618:28)
      at ZoneDelegate.invoke (node_modules/zone.js/dist/zone.js:386:30)
      at ProxyZoneSpec.onInvoke (node_modules/zone.js/dist/proxy.js:117:43)
      at ZoneDelegate.invoke (node_modules/zone.js/dist/zone.js:385:36)
      at Object.onInvoke (../packages/core/src/zone/ng_zone.ts:326:29)
      at ZoneDelegate.invoke (node_modules/zone.js/dist/zone.js:385:36)
      at Zone.run (node_modules/zone.js/dist/zone.js:143:47)
      at NgZone.run (../packages/core/src/zone/ng_zone.ts:180:50)
      at TestBedViewEngine.createComponent (../packages/core/testing/src/test_bed.ts:622:56)
      at Function.TestBedViewEngine.createComponent (../packages/core/testing/src/test_bed.ts:245:36)
      at libs/cmc/home/feature/src/lib/network-zones/details/table/cmc-network-zone-details-table.component.spec.ts:92:23
      at ZoneDelegate.invoke (node_modules/zone.js/dist/zone.js:386:30)
      at ProxyZoneSpec.onInvoke (node_modules/zone.js/dist/proxy.js:117:43)
      at ZoneDelegate.invoke (node_modules/zone.js/dist/zone.js:385:36)
      at Zone.run (node_modules/zone.js/dist/zone.js:143:47)

As far as I researched, a similar issue was closed: #20308, however Angular 10.0.10 doesn't seem to solve this specific issue.

Reproduction

Real world reproduction example:

  1. Clone our open source component library: https://github.com/dynatrace-oss/barista
  2. Install dependencies and build it:
    npm install && npm run build
    The build should pass fine.
  3. Update the @angular/cdk dependency
  4. Re-install dependencies and rebuild it - the build will fail:
ERROR in libs/barista-components/table/src/base-table.ts:64:5 - error TS2554: Expected 8 arguments, but got 7.
64     super(
65       differs,
...
71       platform,
72     );

It can be easily reproduced with a Stackblitz example as well. Please, let me know if you'd like that. Although, I don't think it's necessary - the issue can be easily spotted even with a static analysis.

Expected Behavior

Patch version should not contain API breaking changes.

Actual Behavior

The constructor parameter change causes compilation errors in the library using the CDK directly, and runtime errors for projects using it indirectly by a precompiled dependency.

Environment

  • Angular: 10.0.12
  • CDK/Material: 10.1.3
  • Browser(s): N/A
  • Operating System (e.g. Windows, macOS, Ubuntu): N/A
@szymonbultrowicz szymonbultrowicz added the needs triage This issue needs to be triaged by the team label Aug 26, 2020
@crisbeto
Copy link
Member

Looks like an oversight. We do allow constructor changes in patch versions, but the new parameters have to be optional.

@szymonbultrowicz
Copy link
Author

Hi @crisbeto, thanks for the quick response :)
What would be the action plan? Should we go back to 10.1.1 and wait for a version with an optional parameter?

@crisbeto
Copy link
Member

We'll push some changes to turn it into an optional parameter. It may be easier to wait with upgrading so you don't have to deal with multiple breaking changes.

@crisbeto crisbeto self-assigned this Aug 26, 2020
@crisbeto crisbeto added area: cdk/table has pr P2 The issue is important to a large percentage of users, with a workaround regression This issue is related to a regression and removed needs triage This issue needs to be triaged by the team labels Aug 26, 2020
crisbeto added a commit to crisbeto/material2 that referenced this issue Aug 26, 2020
In angular#19964 and angular#19750 some breaking constructor changes were made which eventually got released as a part of a patch branch which doesn't follow our breaking changes policy.

These changes make the new constructor parameters optional and add fallbacks to the old behavior.

Fixes angular#20422.
crisbeto added a commit to crisbeto/material2 that referenced this issue Aug 26, 2020
In angular#19964 and angular#19750 some breaking constructor changes were made which eventually got released as a part of a patch branch which doesn't follow our breaking changes policy.

These changes make the new constructor parameters optional and add fallbacks to the old behavior.

Fixes angular#20422.
crisbeto added a commit to crisbeto/material2 that referenced this issue Aug 27, 2020
In angular#19964 and angular#19750 some breaking constructor changes were made which eventually got released as a part of a patch branch which doesn't follow our breaking changes policy.

These changes make the new constructor parameters optional and add fallbacks to the old behavior.

Fixes angular#20422.
lukasholzer pushed a commit to dynatrace-oss/barista that referenced this issue Aug 28, 2020
lukasholzer pushed a commit to dynatrace-oss/barista that referenced this issue Aug 28, 2020
crisbeto added a commit to crisbeto/material2 that referenced this issue Sep 3, 2020
In angular#19964 and angular#19750 some breaking constructor changes were made which eventually got released as a part of a patch branch which doesn't follow our breaking changes policy.

These changes make the new constructor parameters optional and add fallbacks to the old behavior.

Fixes angular#20422.
crisbeto added a commit to crisbeto/material2 that referenced this issue Sep 3, 2020
In angular#19964 and angular#19750 some breaking constructor changes were made which eventually got released as a part of a patch branch which doesn't follow our breaking changes policy.

These changes make the new constructor parameters optional and add fallbacks to the old behavior.

Fixes angular#20422.
crisbeto added a commit to crisbeto/material2 that referenced this issue Sep 3, 2020
In angular#19964 and angular#19750 some breaking constructor changes were made which eventually got released as a part of a patch branch which doesn't follow our breaking changes policy.

These changes make the new constructor parameters optional and add fallbacks to the old behavior.

Fixes angular#20422.
crisbeto added a commit to crisbeto/material2 that referenced this issue Sep 3, 2020
In angular#19964 and angular#19750 some breaking constructor changes were made which eventually got released as a part of a patch branch which doesn't follow our breaking changes policy.

These changes make the new constructor parameters optional and add fallbacks to the old behavior.

Fixes angular#20422.
mmalerba pushed a commit to mmalerba/components that referenced this issue Sep 5, 2020
In angular#19964 and angular#19750 some breaking constructor changes were made which eventually got released as a part of a patch branch which doesn't follow our breaking changes policy.

These changes make the new constructor parameters optional and add fallbacks to the old behavior.

Fixes angular#20422.
mmalerba pushed a commit that referenced this issue Sep 5, 2020
In #19964 and #19750 some breaking constructor changes were made which eventually got released as a part of a patch branch which doesn't follow our breaking changes policy.

These changes make the new constructor parameters optional and add fallbacks to the old behavior.

Fixes #20422.
mmalerba pushed a commit that referenced this issue Sep 5, 2020
In #19964 and #19750 some breaking constructor changes were made which eventually got released as a part of a patch branch which doesn't follow our breaking changes policy.

These changes make the new constructor parameters optional and add fallbacks to the old behavior.

Fixes #20422.
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 6, 2020
wagnermaciel pushed a commit to wagnermaciel/components that referenced this issue Jan 14, 2021
In angular#19964 and angular#19750 some breaking constructor changes were made which eventually got released as a part of a patch branch which doesn't follow our breaking changes policy.

These changes make the new constructor parameters optional and add fallbacks to the old behavior.

Fixes angular#20422.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: cdk/table P2 The issue is important to a large percentage of users, with a workaround regression This issue is related to a regression
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants