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

Refactor Rifm into useRifm hook and Rifm component #93

Merged
merged 3 commits into from Feb 7, 2020

Conversation

lewisl9029
Copy link
Contributor

@lewisl9029 lewisl9029 commented Feb 4, 2020

To address #38

Happy to add some tests specifically for the hook if desired, but it
didn't feel super vital atm as all the code paths in the hook are exercised
by the existing render props tests.

Also happy to help in updating docs as well.

I have these changes currently published as @lewisl9029/rifm@0.02 in case anyone wants to try it out.

Happy to add some tests specifically for the hook if desired, but it 
didn't feel super vital as all the code paths in the hook are exercised 
by the existing render props tests.
@lewisl9029 lewisl9029 mentioned this pull request Feb 4, 2020
@istarkov
Copy link
Contributor

istarkov commented Feb 5, 2020

+ 100+ bytes for production to just make it as hook. Rifm is very low level and usually (always) is used inside controls, NumberInput etc. So its very different from hooks I use everyday like useState etc.
Here in issues proposal about hooks was from the first day hooks were announced and we have no decision to be or not to be as there is no real use case when hook Rifm version is better vs render props.
Without changing the size of the production build I see nothing bad to accept this now, but size has changed significantly so it should be real - real use case to accept.

@TrySound
Copy link
Contributor

TrySound commented Feb 5, 2020

There is a way to reduce the size. The source of problem here is rest operator { children, ...args } which forces babel to add helper. To prevent this you may not use rest and instead pass props as is to useRifm. Flow will fail. As a workaround cast props to any like this useRifm((props: any )). We do not iterate over props so it's safe to do it in place.

@istarkov
Copy link
Contributor

istarkov commented Feb 5, 2020

@TrySound Can we after somehow measure the impact size of using hook only version having that Rifm export could be treeshaked? As with suggestion above hook only version can be smaller vs render props?

@TrySound
Copy link
Contributor

TrySound commented Feb 5, 2020

843 -> 834

@lewisl9029
Copy link
Contributor Author

lewisl9029 commented Feb 5, 2020

Great catch! I tried @TrySound's suggestion and it reduced the gzipped size to 861, an 18 bytes increase. Is that more acceptable @istarkov?

I think it might also be worthwhile to consider a v2 that provides only a hook, since it's fairly trivial for users to build a Rifm render props component from a useRifm hook (i.e. the 2 lines implemented here), but as a user, I personally couldn't figure out any good ways to convert the Rifm component into a hook, hence this PR. But that's a much bigger decision that's outside of the scope of this PR. :)

@TrySound
Copy link
Contributor

TrySound commented Feb 6, 2020

Hook only will be probably v1. But we will keep render prop for now to make soft upgrade without rewriting everything.

@istarkov
Copy link
Contributor

istarkov commented Feb 6, 2020

Great!
So hook version tests left. Everything here is covered with tests.
Additional tests are:
Copy paste of
RifmFormat.test.js -> + RifmFormatHook.test.js
RifmMask.test.js -> + RifmMaskHook.test.js

And additional createExec method createExecHook with hook inside instead of render function
(Same as createExec just with hook)
https://github.com/realadvisor/rifm/blob/master/tests/utils/exec.js#L23

@istarkov
Copy link
Contributor

istarkov commented Feb 6, 2020

Also it will be good to add few hook version examples

@TrySound
Copy link
Contributor

TrySound commented Feb 6, 2020

@istarkov I would just refactor original tests with hook version. Render prop is a thing wrapper without any logic.

@istarkov
Copy link
Contributor

istarkov commented Feb 6, 2020

Possibly you are right, I have no strong opinion on this

@TrySound
Copy link
Contributor

TrySound commented Feb 6, 2020

@lewisl9029 Upgrade readme with hook version please and we will merge.

@lewisl9029
Copy link
Contributor Author

@TrySound @istarkov I gave it a shot here: e1b3846

Let me know what y'all think, and feel free to make any edits you feel is appropriate.

@TrySound TrySound merged commit ffda0bb into realadvisor:master Feb 7, 2020
@TrySound
Copy link
Contributor

TrySound commented Feb 7, 2020

thanks!

@lewisl9029 lewisl9029 deleted the rifm-hook branch February 7, 2020 18:22
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.

None yet

3 participants