feat: headless functionality in hook

bhubr / react-simple-oauth2-login

Simple React component for OAuth2 login - Supports Authorization Code and Implicit Grant flows.

MIT License

49 stars 31 forks source link

feat: headless functionality in hook #45

Open jshthornton opened 3 years ago

jshthornton commented 3 years ago

This pull request moves the functionality of the oauth logic into a hook. That way this package can be utilised headlessly allowing developers to manage the UI aspect.

I've put this PR into draft until the following have been done:

[x] Maintainers are happy with the concept of moving to headless (whilst still providing a component for ease of use)
[ ] Whether this is classified as a breaking change as it utilises hooks which were introduced into React v16.8.0
[ ] Tests have been written exclusively for the hook
[ ] Docs have been updated to include the hook usage

bhubr commented 3 years ago

Hello!

Well, thanks for the work you put in!

I initially made this package for my own use. Since then, it seems to have taken a life of its own. Sure, there are not that many users, but I'd need to make sure that we don't break their apps: either by bumping a major version, and/or verifying that they' re using recent React versions (which seems to be the case, by picking a few random repos that use it).
I actually hadn't given any thought about going headless. Could you please provide some more background (a use case or an example)? Is it mainly for making tests easier, or is there another aspect to it that I'm totally missing? (sorry if it's a very naive question!)

bhubr commented 3 years ago

By the way, I also have quite a few issues left hanging: mainly #16, #22, #33, #37

So I'm thinking it would be a good moment to address at least some of them, and to include this PR into the overall plan. So that we move forward to 1.0 with both new features & under-the-hood changes.

jshthornton commented 3 years ago

tbh it isn't even a breaking change, as whilst this PR uses the hooks syntax of use* there isn't actually any hooks utilised under the hood, so versions < v16.8.0 would still work.

The benefits of going headless are that it decouples the functionality from the presentation, allowing for easier testing, as well as for consuming apps to truly implement the rendering component as desired. In the app I am using this package with I'm having to use the render function instead to get customised DOM to render icons, data-test-id as well as to apply some custom styles on the fly instead of class names.

jshthornton commented 3 years ago

I'm happy to help contribute on some of those other features / bugs too when I get a spare moment.

bhubr commented 3 years ago

Thanks for the additional context! I'm pretty busy today but I'll answer more thoroughly as soon as I can (incl. on your latest issue, which I find interesting!)

bhubr commented 2 years ago

Hi @jshthornton

Sorry it took me so long to respond. You probably moved on to other things since then! I started a new job which kept me awfully busy. The pace is slowing down a bit, so I intend to resume work on the library.

After a quick glance at what you did already, it seems good to me! I'm gonna try and write tests for it.

DiefBell commented 2 years ago

Has any more work been done on this PR? A hook would be great because I'd then be able to quite easily hook it up to https://www.npmjs.com/package/react-social-login-buttons

bhubr commented 2 years ago

@DiefBell I've been (and still am) super-super-busy and haven't done more than manually testing. I expect to finish a fairly big project in about a month. If you have skills in writing tests, you're more than welcome to contribute!

Other options:

install module with npm using repo name and branch
clone jshthornton's fork, setup a private npm registry with Verdaccio, publish to this local registry, and npm install from this registry

Sorry, I really can't devote even the tiniest amount of time to this project right now!

bhubr commented 1 year ago

Hi @jshthornton

Sorry for coming back way after due time... I'm only finding time and motivation now to resume working on the project. Since it's one of the biggest changes and I have other plans ahead, I think it's better that I proceed with this issue first.

I pulled your branch and rebased it on the current master branch. I pushed the rebased branch here: https://github.com/bhubr/react-simple-oauth2-login/tree/feature/52-pr45-jshthornton-headless-functionality-in-hook

Are you OK with me continuing from there?