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

feat: Add option to check takeUntil within all angular entities #116

Merged
merged 2 commits into from
May 12, 2020

Conversation

vakhaniya
Copy link
Contributor

Added option to check takeUntil within services, directives, pipes and components

Services, directives and pipes may also contain subscriptions to unsubscribe from when they are destroyed. They also can implement OnDestroy

This is especially important in case of microfrontend when several apps can be bootstraped and destroyed while page lifetime

@cartant
Copy link
Owner

cartant commented May 8, 2020

I've had a quick look at this and it LGTM. I'll review it tomorrow. Thanks.

@cartant cartant self-requested a review May 8, 2020 13:52
Copy link
Owner

@cartant cartant left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks.

I do have one small suggestion: could you rename checkAllClasses to checkDecorators and have it accept an array of decorator names?

"checkDecorators": ["Component"] // The default, if the option isn't specified
"checkDecorators": ["All"] // Match/check all of the decorators in your PR
"checkDecorators": ["Component", "Pipe"] // Match/check only these

IMO, this would better represent how checked classes are matched and would potentially avoid confusion that could arise from @Injectable not strictly being required.

Copy link
Owner

@cartant cartant left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

@cartant cartant merged commit 533eb63 into cartant:master May 12, 2020
@cartant
Copy link
Owner

cartant commented May 12, 2020

4.32.0 should be published now.

@vakhaniya
Copy link
Contributor Author

Thanks for review and help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants