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.

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

update abb patch #3977

Open
wants to merge 2 commits into
base: abb
Choose a base branch
from
+17 −3
Open

update abb patch #3977

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f43d1fa
Copy patch of https://github.com/preactjs/preact/pull/3968
jpnelson Apr 21, 2023
69f9408
Patch to make unmounting faster. PR TBD
jpnelson Apr 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 13 additions & 2 deletions src/clone-element.js
View file
Open in desktop
Expand Up @@ -13,10 +13,21 @@ export function cloneElement(vnode, props, children) {
key,
ref,
i;

let defaultProps;

if (vnode.type && vnode.type.defaultProps) {
defaultProps = vnode.type.defaultProps;
}

for (i in props) {
if (i == 'key') key = props[i];
else if (i == 'ref') ref = props[i];
else normalizedProps[i] = props[i];
else if (props[i] === undefined && defaultProps !== undefined) {
normalizedProps[i] = defaultProps[i];
} else {
normalizedProps[i] = props[i];
}
}

if (arguments.length > 2) {
Expand All @@ -31,4 +42,4 @@ export function cloneElement(vnode, props, children) {
ref || vnode.ref,
null
);
}
}
5 changes: 4 additions & 1 deletion src/create-context.js
View file
Open in desktop
Expand Up @@ -52,7 +52,10 @@ export function createContext(defaultValue, contextId) {
subs.push(c);
let old = c.componentWillUnmount;
c.componentWillUnmount = () => {
subs.splice(subs.indexOf(c), 1);
// Patch: this is a hot path, and taking sub out of the array is much
// faster this way since it's an unordered list.
subs[subs.indexOf(c)] = subs[subs.length - 1];
subs.pop();
Comment on lines +55 to +58
Copy link
Member

Choose a reason for hiding this comment

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

Can you also open a PR for against to master? Happy to do that as well if you are busy :)

Copy link
Author

Choose a reason for hiding this comment

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

Yes for sure! #3979

Comment on lines +57 to +58
Copy link
Member

Choose a reason for hiding this comment

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

not sure if smaller/faster/slower:

Suggested change
subs[subs.indexOf(c)] = subs[subs.length - 1];
subs.pop();
subs[subs.indexOf(c)] = subs.pop();

Copy link
Author

Choose a reason for hiding this comment

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

I haven't tested it but I can't see how that'd be any different from having a separate assignment (maybe it'd even get minified to the same thing?) so that's the version I've put into the PR here: #3979

Copy link
Member

Choose a reason for hiding this comment

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

I think this suggestion fails if you are removing the last item in the array

Copy link
Member

Choose a reason for hiding this comment

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

indeed it does!

if (old) old.call(c);
};
};
Expand Down