Skip to content

Move hooks to core#2203

Closed
JoviDeCroock wants to merge 12 commits into
masterfrom
hooks-in-core
Closed

Move hooks to core#2203
JoviDeCroock wants to merge 12 commits into
masterfrom
hooks-in-core

Conversation

@JoviDeCroock

@JoviDeCroock JoviDeCroock commented Dec 22, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Before:

  • hooks: 891
  • core: 3629

Total: 4520B

After:

  • hooks: 171
  • core: 4308

Total: 4479B

After (useDebug and useImperative to /hooks):

  • hooks: 251
  • core: 4229

Total: 4480B

Note: this has to be refactored for useErrorBoundary but I'm gong to wait for more comments on this. If we decide to expand functionality of hooks we should be wary of this (since size++)

@coveralls

coveralls commented Dec 22, 2019 •
edited
Loading

Copy link
Copy Markdown

Coverage Status

Coverage increased (+1.5%) to 100.0% when pulling 2b0046d on hooks-in-core into 54c0f02 on master.

@JoviDeCroock JoviDeCroock mentioned this pull request Dec 22, 2019
Merged
porfirioribeiro
porfirioribeiro reviewed Jan 3, 2020
View reviewed changes

@porfirioribeiro porfirioribeiro left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It seems nice!
With this change we align Preact more with React, that is good in one side because it can remove overhead and make overall code smaller for Preact+hooks usage.
In other hand, it brings hooks logic into core, that might not be used in some cases

Comment thread src/hooks.js Outdated
@JoviDeCroock

JoviDeCroock commented Jan 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

In other hand, it brings hooks logic into core, that might not be used in some cases

This should be close to a noop (maybe 50-100B at max) since when unused that whole file can be tree-shaken

@porfirioribeiro

porfirioribeiro commented Jan 3, 2020

Copy link
Copy Markdown

In other hand, it brings hooks logic into core, that might not be used in some cases

This should be close to a noop (maybe 50B at max) since when unused that whole file can be tree-shaken

Ohh yeah, that's great then!
Maybe i will even be able to use some of that logic on composition side

@JoviDeCroock JoviDeCroock deleted the hooks-in-core branch August 21, 2020 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@porfirioribeiro porfirioribeiro porfirioribeiro left review comments

@developit developit Awaiting requested review from developit

@andrewiggins andrewiggins Awaiting requested review from andrewiggins

@marvinhagemeister marvinhagemeister Awaiting requested review from marvinhagemeister

@cristianbote cristianbote Awaiting requested review from cristianbote

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JoviDeCroock @coveralls @porfirioribeiro