GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — fix(compiler-cli): libraries compiled with v16.1+ breaking with framework v16.0.x apps by devversion · Pull Request #50714 · angular/angular

Conversation

devversion commented Jun 14, 2023

Copy link

Copy Markdown

Member

If a library is compiling with Angular v16.1.0, the library will break for users that are still on Angular v16.0.x. This happens because the DirectiveDeclaration or ComponentDeclaration types are not expecting an extra field for signals metadata. This field was only added to the generic types in 16.1.0- so compilations fail with errors like this:

Error: node_modules/@angular/material/icon/index.d.ts:204:18 -
  error TS2707: Generic type 'ɵɵComponentDeclaration' requires between 7 and 9 type arguments.

To fix this, we quickly roll back the code for inserting this metadata field. That way, libraries remain compatible with all v16.x framework versions.

We continue to include the signals metadata if signals: true is set. This is not public API anyway right now- so cannot happen- but imagine we expose some signal APIs in e.g. 16.2.x, then we'd need this metadata and can reasonably expect signal-component library users to use a more recent framework core version.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          fix(compiler-cli): libraries compiled with v16.1+ breaking with Angul…

3cbaf39

…ar framework v16.0.x

If a library is compiling with Angular v16.1.0, the library will break
for users that are still on Angular v16.0.x. This happens because the
`DirectiveDeclaration` or `ComponentDeclaration` types are not expecting
an extra field for `signals` metadata. This field was only added to the
generic types in `16.1.0`- so compilations fail with errors like this:

```
Error: node_modules/@angular/material/icon/index.d.ts:204:18 -
  error TS2707: Generic type 'ɵɵComponentDeclaration' requires between 7 and 9 type arguments.
```

To fix this, we quickly roll back the code for inserting this metadata
field. That way, libraries remain compatible with all v16.x framework
versions.

We continue to include the `signals` metadata if `signals: true` is set.
This is not public API anyway right now- so cannot happen- but imagine
we expose some signal APIs in e.g. 16.2.x, then we'd need this metadata
and can reasonably expect signal-component library users to use a more
recent framework core version.

devversion requested review from alxhub and crisbeto

June 14, 2023 12:36

devversion added the area: compiler Issues related to `ngc`, Angular's template compiler label

Jun 14, 2023

ngbot Bot added this to the Backlog milestone

Jun 14, 2023

devversion added action: review

The PR is still awaiting reviews from at least one requested reviewer

target: patch This PR is targeted for the next patch release labels

Jun 14, 2023

crisbeto approved these changes

Jun 14, 2023

View reviewed changes

crisbeto left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

packages/compiler/src/render3/view/compiler.ts

		// TODO(signals): Always include this metadata starting with v17. Right
		// now Angular v16.0.x does not support this field and library distributions
		// would then be incompatible with v16.0.x framework users.
		if (meta.isSignal) {

crisbeto Jun 14, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'm not sure we'd even need to revert this until we add another parameter after it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

devversion Jun 14, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, although I think making all optional fields required over time is beneficial as we can then introduce required fields in the future, without having to re-order.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

devversion removed the request for review from alxhub

June 14, 2023 12:46

devversion added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels

Jun 14, 2023

devversion marked this pull request as ready for review

June 14, 2023 12:46

devversion mentioned this pull request

Jun 14, 2023

[16.1.x]: fix(compiler-cli): libraries compiled with v16.1+ breaking with framework v16.0.x #50715

Closed

devversion added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels

Jun 14, 2023


          fixup! fix(compiler-cli): libraries compiled with v16.1+ breaking wit…

1ec3003

…h Angular framework v16.0.x

Update golden

pullapprove Bot requested review from alxhub, dylhunn and jessicajaniuk

June 14, 2023 13:01

pkozlowski-opensource requested changes

Jun 14, 2023

View reviewed changes

pkozlowski-opensource left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it mean "forward compatibility"? I mean, we don't have guarantees for libraries compiled with 16.1 when it comes to working with 16.0. Things might work by accident, but I don't think we should go out of our ways to support this situation. Doing so would send a precedent / expectations and I'm not sure we want to find ourselves there.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pkozlowski-opensource approved these changes

Jun 14, 2023

View reviewed changes

pkozlowski-opensource left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussing it further we want to maintain backward compatibility across the same set of major versions.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

alxhub approved these changes

Jun 14, 2023

View reviewed changes

alxhub left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-for: global-approvers

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pkozlowski-opensource removed request for dylhunn and jessicajaniuk

June 14, 2023 13:51

pkozlowski-opensource commented Jun 14, 2023

Copy link

Copy Markdown

Member

This PR was merged into the repository by commit 12bad65.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pkozlowski-opensource closed this in

12bad65

Jun 14, 2023

angular-automatic-lock-bot Bot commented Jul 15, 2023

Copy link

Copy Markdown

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.}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

angular-automatic-lock-bot Bot locked and limited conversation to collaborators

Jul 15, 2023

ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request

Jan 23, 2024


          fix(compiler-cli): libraries compiled with v16.1+ breaking with Angul…

cf5a69a

…ar framework v16.0.x (angular#50714)

If a library is compiling with Angular v16.1.0, the library will break
for users that are still on Angular v16.0.x. This happens because the
`DirectiveDeclaration` or `ComponentDeclaration` types are not expecting
an extra field for `signals` metadata. This field was only added to the
generic types in `16.1.0`- so compilations fail with errors like this:

```
Error: node_modules/@angular/material/icon/index.d.ts:204:18 -
  error TS2707: Generic type 'ɵɵComponentDeclaration' requires between 7 and 9 type arguments.
```

To fix this, we quickly roll back the code for inserting this metadata
field. That way, libraries remain compatible with all v16.x framework
versions.

We continue to include the `signals` metadata if `signals: true` is set.
This is not public API anyway right now- so cannot happen- but imagine
we expose some signal APIs in e.g. 16.2.x, then we'd need this metadata
and can reasonably expect signal-component library users to use a more
recent framework core version.

PR Close angular#50714

Reviewers

pkozlowski-opensource

pkozlowski-opensource approved these changes

alxhub

alxhub approved these changes

crisbeto

crisbeto approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

action: merge

The PR is ready for merge by the caretaker

area: compiler

Issues related to `ngc`, Angular's template compiler

target: minor

This PR is targeted for the next minor release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.