Skip to content

fix(compiler-cli): strictMetadataEmit should not break on non-compliant libraries #23275

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

Closed
wants to merge 1 commit into from

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Apr 9, 2018

rxjs 6.0.0 breaks strictMetadataEmit as they now publish a .d.ts file with a
structure like:

declare export class Subscription {
static EMPTY: Subscription;
}

This generates metadata which contains an error, and fails the strictMetadataEmit
validation. There is nothing a library author can do in this situation except to
set strictMetadataEmit to false.

The spirit of strictMetadataEmit is to validate that the author's library doesn't
do anything that will break downstream users. This failure is a corner case which
causes more harm than good, so this commit disables validation for metadata
collected from .d.ts files.

…nt libraries

rxjs 6.0.0 breaks strictMetadataEmit as they now publish a .d.ts file with a
structure like:

declare export class Subscription {
  static EMPTY: Subscription;
}

This generates metadata which contains an error, and fails the strictMetadataEmit
validation. There is nothing a library author can do in this situation except to
set strictMetadataEmit to false.

The spirit of strictMetadataEmit is to validate that the author's library doesn't
do anything that will break downstream users. This failure is a corner case which
causes more harm than good, so this commit disables validation for metadata
collected from .d.ts files.

Fixes angular#22210
@mary-poppins
Copy link

You can preview a9e998d at https://pr23275-a9e998d.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 729409b at https://pr23275-729409b.ngbuilds.io/.

@mhevery mhevery added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Apr 9, 2018
@IgorMinar IgorMinar added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Apr 9, 2018
@IgorMinar
Copy link
Contributor

This PR doesn't apply cleanly to the stable branch so I'm going to merge it just to master.

@IgorMinar IgorMinar closed this in 5814355 Apr 9, 2018
@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 Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants