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

use options.unmount instead of overriding component.componentWillUnmount #2919

Merged
merged 2 commits into from
Feb 9, 2021

Conversation

tanhauhau
Copy link
Contributor

@tanhauhau tanhauhau commented Jan 11, 2021

Following up to #2917, use options.unmount instead of overriding componentWillUnmount

this will check component._onResolve on every unmount though, not sure how much will this add up to slow things down

@coveralls
Copy link

coveralls commented Jan 11, 2021

Coverage Status

Coverage increased (+0.003%) to 99.445% when pulling 0802e6d on tanhauhau:tanlh/use-unmount-options into 3efb2d0 on preactjs:master.

@developit
Copy link
Member

From the size bot:

Size Change: +29 B (0%)

Total Size: 42 kB

Filename Size Change
compat/dist/compat.js 3.35 kB +10 B (0%)
compat/dist/compat.module.js 3.34 kB +9 B (0%)
compat/dist/compat.umd.js 3.4 kB +10 B (0%)

@tanhauhau tanhauhau force-pushed the tanlh/use-unmount-options branch from 76100c5 to 92664b9 Compare February 9, 2021 07:12
Copy link
Contributor Author

@tanhauhau tanhauhau left a comment

Choose a reason for hiding this comment

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

I've added some test cases and relevant fixes from #2989

@tanhauhau tanhauhau force-pushed the tanlh/use-unmount-options branch from 92664b9 to 5fffc2c Compare February 9, 2021 07:16
@tanhauhau tanhauhau force-pushed the tanlh/use-unmount-options branch from 5fffc2c to 0802e6d Compare February 9, 2021 07:26
@JoviDeCroock JoviDeCroock merged commit 9250ac9 into preactjs:master Feb 9, 2021
@JoviDeCroock
Copy link
Member

Thank you so much for all the efforts @tanhauhau

This was referenced Mar 15, 2021
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.

5 participants