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

Revert pull request #37 #57

Merged
merged 8 commits into from
Nov 8, 2020
Merged

Revert pull request #37 #57

merged 8 commits into from
Nov 8, 2020

Conversation

bertho-zero
Copy link
Collaborator

No description provided.

@bertho-zero
Copy link
Collaborator Author

Hi @daffl, as discussed in #37 (comment) I have put the params/props management back in the HookManager while keeping the fixes made since.

I still have to add the options and see some easy performance improvements.

@bertho-zero
Copy link
Collaborator Author

bertho-zero commented Oct 29, 2020

I just set the defaults of props and params to null rather than an empty array/object, and then avoid unnecessary concat and assign calls.

I added a log in the benchmark tests before/after these changes:

Before:

Runtime: 13
Runtime: 127
Runtime: 149
Runtime: 235

After:

Runtime: 13
Runtime: 89 (-30%)
Runtime: 97 (-35%)
Runtime: 172 (-27%)

@daffl
Copy link
Member

daffl commented Oct 29, 2020

Great, thank you, I've been meaning to do this for a while. I'll have a look at it this weekend 😄

@bertho-zero bertho-zero marked this pull request as draft October 29, 2020 15:51
@bertho-zero bertho-zero marked this pull request as ready for review October 29, 2020 17:35
@bertho-zero
Copy link
Collaborator Author

@daffl
Copy link
Member

daffl commented Nov 8, 2020

Great, thank you for doing that. Also, really nice to see the performance improvements.

@daffl daffl merged commit 72f427a into master Nov 8, 2020
@daffl daffl deleted the manager branch November 8, 2020 19:40
daffl pushed a commit that referenced this pull request Nov 12, 2020
BREAKING CHANGES: This reverts back to the usage of v0.4.0 and before.
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