Skip to content

build: add CI check to ensure consistent exports for MDC packages #20960

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 12, 2020
Merged

build: add CI check to ensure consistent exports for MDC packages #20960

merged 1 commit into from
Nov 12, 2020

Conversation

@crisbeto
Copy link
Member

@crisbeto crisbeto commented Nov 3, 2020

Adds a new script that checks whether an MDC package exports the same set of symbols as its non-MDC counterpart. Also fixes all of the failures.

@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 3, 2020
crisbeto
crisbeto commented Nov 3, 2020
View reviewed changes
@crisbeto crisbeto force-pushed the mdc-symbols-check branch 2 times, most recently from ce9c1d0 to f0149c8 Compare November 3, 2020 17:00
@crisbeto crisbeto marked this pull request as ready for review November 3, 2020 17:10
@crisbeto crisbeto added merge safe P2 The issue is important to a large percentage of users, with a workaround target: major This PR is targeted for the next major release labels Nov 3, 2020
mmalerba
mmalerba reviewed Nov 5, 2020
View reviewed changes
src/material-experimental/mdc-form-field/public-api.ts
export {
MAT_FORM_FIELD,
MatFormFieldControl,
getMatFormFieldDuplicatedHintError,
Copy link
Contributor

@mmalerba mmalerba Nov 5, 2020

Choose a reason for hiding this comment

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

Hmm, these don't have an underscore but they are @docs-private. They really shouldn't have been part of the public API... @jelbourn do you think we can skip re-exporting @docs-private stuff?

Copy link
Member Author

@crisbeto crisbeto Nov 5, 2020

Choose a reason for hiding this comment

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

I think that we have things like this in a bunch of packages, but we just have to live with it and eventually deprecate them.

Copy link
Contributor

@mmalerba mmalerba Nov 5, 2020

Choose a reason for hiding this comment

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

Same, if we decide to keep these we should at least move them under a TODO to get rid of them later

Copy link
Member

@jelbourn jelbourn Nov 6, 2020

Choose a reason for hiding this comment

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

Yeah, at this point anything without an underscore would have to go through the normal process. I would like to explore removing "library internal" symbols from our npm packages via transformation at some point, but that's probably not going to happen soon.

@crisbeto
Copy link
Member Author

crisbeto commented Nov 6, 2020

@mmalerba I've added a config file to allow for tests to be skipped and excluded all of the underscored classes.

@crisbeto crisbeto force-pushed the mdc-symbols-check branch 2 times, most recently from ef6d232 to f806435 Compare November 6, 2020 06:22
mmalerba
mmalerba approved these changes Nov 6, 2020
View reviewed changes
Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

LGTM

@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label Nov 6, 2020
jelbourn
jelbourn reviewed Nov 6, 2020
View reviewed changes
src/material-experimental/mdc-form-field/public-api.ts
export {
MAT_FORM_FIELD,
MatFormFieldControl,
getMatFormFieldDuplicatedHintError,
Copy link
Member

@jelbourn jelbourn Nov 6, 2020

Choose a reason for hiding this comment

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

Yeah, at this point anything without an underscore would have to go through the normal process. I would like to explore removing "library internal" symbols from our npm packages via transformation at some point, but that's probably not going to happen soon.

@crisbeto crisbeto force-pushed the mdc-symbols-check branch 2 times, most recently from ec2c9c7 to a62d816 Compare November 6, 2020 21:26
jelbourn
jelbourn approved these changes Nov 9, 2020
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@crisbeto
build: add CI check to ensure consistent exports for MDC packages
34b2be1 
Adds a new script that checks whether an MDC package exports the same set of
symbols as its non-MDC counterpart. Also fixes all of the failures.
@mmalerba mmalerba added target: minor This PR is targeted for the next minor release and removed cla: yes PR author has agreed to Google's Contributor License Agreement target: major This PR is targeted for the next major release labels Nov 12, 2020
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 12, 2020
@mmalerba mmalerba added action: merge The PR is ready for merge by the caretaker and removed action: merge The PR is ready for merge by the caretaker labels Nov 12, 2020
@mmalerba mmalerba merged commit ae061d7 into angular:master Nov 12, 2020
@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Dec 13, 2020

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 Dec 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jelbourn jelbourn jelbourn approved these changes

@mmalerba mmalerba mmalerba approved these changes

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

@devversion devversion Awaiting requested review from devversion

Assignees

@jelbourn jelbourn

@mmalerba mmalerba

Labels

action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@crisbeto @jelbourn @mmalerba