Skip to content

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

Merged
JoviDeCroock merged 2 commits intopreactjs:masterfrom
tanhauhau:tanlh/use-unmount-options
Feb 9, 2021
Merged

use options.unmount instead of overriding component.componentWillUnmount#2919
JoviDeCroock merged 2 commits intopreactjs:masterfrom
tanhauhau:tanlh/use-unmount-options

Conversation

@tanhauhau
Copy link
Copy Markdown
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

Comment thread src/internal.d.ts Outdated
Comment thread compat/src/suspense.js
@coveralls
Copy link
Copy Markdown

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.

Comment thread compat/src/suspense.js Outdated
@developit
Copy link
Copy Markdown
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%)

Comment thread compat/src/suspense.js Outdated
@tanhauhau tanhauhau force-pushed the tanlh/use-unmount-options branch from 76100c5 to 92664b9 Compare February 9, 2021 07:12
Copy link
Copy Markdown
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

Comment thread compat/src/suspense.js Outdated
@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
Copy Markdown
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