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

Fix the problem on destroy happening to fast #111

Merged
merged 8 commits into from
Jun 25, 2020
Merged

Conversation

bhugot
Copy link
Contributor

@bhugot bhugot commented Jun 4, 2020

The problem is visible in #110

@bhugot
Copy link
Contributor Author

bhugot commented Jun 5, 2020

Could be fixing also #38

@f1ames f1ames self-requested a review June 9, 2020 08:52
@f1ames f1ames self-assigned this Jun 9, 2020
@f1ames f1ames changed the base branch from master to stable June 9, 2020 09:01
@f1ames f1ames changed the base branch from stable to master June 9, 2020 09:02
Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @bhugot!

I have checked your PR and it looks quite good 👍 Did some minor refactoring. See also one tests related comment.

Looking at your example from #110 I wonder what will happen if component will be shown again (might be the case described in #38). Will the components used inside (so ckeditor one) be initialized as new instances? I'm wondering if _destroyed flag state will be correct in such cases 🤔

Could be fixing also #38

As you also mentioned it, would you be willing to add unit test covering this case also (so probably the same thing I described above too)?

src/ckeditor/ckeditor.component.spec.ts Outdated Show resolved Hide resolved
@bhugot
Copy link
Contributor Author

bhugot commented Jun 9, 2020

About the #38 as we don't even call create editor we should not have this problem

@bhugot
Copy link
Contributor Author

bhugot commented Jun 10, 2020

Be aware that there is still a case where it will be failling if the component is in a ng-content as the ngOnDrestroy is not called https://stackblitz.com/edit/angular-ivy-jvybwy?file=src%2Fapp%2Fapp.component.html

@bhugot bhugot requested a review from f1ames June 10, 2020 11:43
@f1ames
Copy link
Contributor

f1ames commented Jun 18, 2020

@ezintz Thank you for checking this PR!

Be aware that there is still a case where it will be failling if the component is in a ng-content as the ngOnDrestroy is not called https://stackblitz.com/edit/angular-ivy-jvybwy?file=src%2Fapp%2Fapp.component.html

@bhugot can you elaborate more on this one? I don't see any errors in your code sample (nor CKEditor component)?

@bhugot
Copy link
Contributor Author

bhugot commented Jun 18, 2020

this example only show that when you have a as parent of another component then even if not displayed the ngOnInit is call (look in console log 'Child initialized') but the onDestroy is not call. That can lead on error with CKEditor (it's not an issue with ckeditor-angular) just for reference if someone has this issue sometime after this PR is merged

@f1ames
Copy link
Contributor

f1ames commented Jun 19, 2020

this example only show that when you have a as parent of another component then even if not displayed the ngOnInit is call (look in console log 'Child initialized') but the onDestroy is not call. That can lead on error with CKEditor (it's not an issue with ckeditor-angular) just for reference if someone has this issue sometime after this PR is merged

Thanks for the clarification @bhugot 👍 So it seems this is an issue in Angular itself (btw. do you have any number/reference to this issue, was it reported somewhere)?

Do you know any workaround for this issue so we can keep it for future reference?

@bhugot
Copy link
Contributor Author

bhugot commented Jun 19, 2020

An issue on this : angular/angular#18982
One way to handle it, is to use a ng-template so the template is only used when is parent is visible

@f1ames f1ames self-assigned this Jun 25, 2020
@f1ames
Copy link
Contributor

f1ames commented Jun 25, 2020

Rebased onto latest master.

Copy link
Contributor

@f1ames f1ames 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 for contributing @bhugot!

@bhugot
Copy link
Contributor Author

bhugot commented Jun 25, 2020

You're welcome

@f1ames f1ames merged commit d3ceee6 into ckeditor:master Jun 25, 2020
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.

None yet

3 participants