volatiletech / authboss

The boss of http auth.
MIT License
3.81k stars 207 forks source link

Revamping Authboss!!! #305

Closed ibraheemdev closed 4 years ago

ibraheemdev commented 4 years ago

Unfortunately, when new users come to this repository, the cannot see how amazing it is due to lack of documentation and content. This needs to change. I forked the repo and made major experimental edits. Here is a list of the changes I made that I think would greatly improve the way the community sees this project. I can create pull requests for the ones that you approve of.

Please accept this request. I want authboss to be the devise of golang. But to accomplish that, we must have documentation, resources, content, defaults everything, except the server store, and generators.

To view all the updates I made, you can view my repo

aarondl commented 4 years ago

Hi @ibraheemdev! I'm glad you're interested in improving Authboss.

Many of the changes you've created are about merging pieces into the main repo of Authboss. The Go ecosystem's dependency trees are approaching NPM levels and we should be doing everything we can to stop this. There is no reason this project should have to depend on the Gorilla Toolkit and seeing this as a dependency in my projects would frustrate me to no end because I do not use Gorilla and do not want Go constantly downloading it just to look at it's go.mod and throw it in the garbage.

To that end no change that brings additional unnecessary dependencies into Authboss will be accepted.

As for the sample application the point of it was never to show the most basic things about Authboss, it was meant to show as much as humanly possible about it integrated in a "real world" application. The sample that you have included in your repository is missing a great deal of what Authboss is capable of and doesn't give users ideas on how to integrate all the modules together. Yes it's complicated, but Authboss is not an easy library to deal with (unfortunately).

The abrenderer is complicated because it deals with both html and text rendering for both pages and e-mails. It additionally uses bindata because in the past there's been a great many issues stemming from assumptions about where templates are supposed to live on disk in my many generation projects. The default renderer you've created doesn't correctly understand text templates, it only does HTML so this unfortunately disqualifies it as a candidate to be a goto default renderer for Authboss. If we fixed this, I'm opened to it being a low-power renderer in the defaults package.

The generators I'm pretty indifferent towards. I don't think they're really necessary, but they're not completely bad either. The main thing I don't like is that now we have an authboss binary whose only purpose is to copy files around (and yet more dependencies). Why couldn't we just have a directory with these sample files in them and docs that point to them?

As far as making expire side-effect imported, the project is moving away from these. The 2fa modules for example do not use side effect imports and in a breaking change in the next major version the intention is for no side effect imports to remain at all so the change to expire should likely be undone.

The docs are indeed pretty, but I've always been personally averse to docs sites. I don't understand their purpose. As you say it's just a pile of Markdown but that's what the README is. The only thing we gain is a sidebar but the price we pay is now to edit docs instead of editing a markdown file we now additionally have to run some build process to generate the HTML and put it in our docs (probably including node.js because that dependency always wants to be installed for some reason or another). I don't like the idea of having to remember/build CI around regenerating documentation when we can just edit a text file as is being done currently and there's no actual problems. Prettier? Yes. Slightly more navigatable due to sidebar? Yes. But a much bigger hassle for the development side.

Lastly, I'm not really interested in attracting many more users to my libraries. I've decided that Open Source is quite a lot of labor and takes a great deal of time. I'm happy to continue to maintain them, fixing bugs and making sure things work for people but to do this huge amount of rework with the bottom line being: "Let's make it more accessible so people use it/the community views Authboss in a better light!" doesn't seem like it meshes with how I want things to go since that would increase the user base and I don't see a need for that. The project works for myself and others currently.

So in summary:

I'm 100% sure that this is not the response you were looking for. You definitely seem eager and motivated to make this project better and obviously put a lot of time and effort into this. But this is why I usually tell people to discuss large changes with maintainers beforehand. People have a lot of different ideas of what "better" means. Interested to hear your thoughts and what you plan to do.

ibraheemdev commented 4 years ago

I see what you mean. I'm not opposed to anything you said. Everything I did on my fork was for use in my personal projects, so there is no harm done 🙂 One suggestion I have is that you should remove the friends of go error library as a dependency. I already did this, and can create a pull request if you wish. It is simply a wrapper around fmt ErrorF. Thanks!

I will create pull requests for things I updated that don't add new functionality. I will also update the authboss sample app when I get the time, and add a default html renderer that deals with text/template. Would you accept the renderer if it doesn't compile bindata?

ibraheemdev commented 4 years ago

Also, the docsify site is really just markdown. There is no npm build process, just a index.html with a js script tag. Editing the markdown triggers GitHub pages to redeploy the site. Can you reevaluate adding a docs site to the repo?

It's true that node modules introduce a never ending dependency tree, and this is why I chose docsify and not something like gitbook. Screenshot_20200813-204718~2

aarondl commented 4 years ago

One suggestion I have is that you should remove the friends of go error library as a dependency. I already did this, and can create a pull request if you wish. It is simply a wrapper around fmt ErrorF. Thanks!

It does do slightly more than that. Unfortunately when the Go developers created fmt.Errorf and the %w verb they pushed stack traces as a feature out of the proposals because nobody could agree on how to do it. I'd vote we keep the dependency for that reason because it'd be a breaking behavior change to those that expect the stack trace to exist.

and add a default html renderer that deals with text/template. Would you accept the renderer if it doesn't compile bindata?

Yes, my concern isn't bindata, just that it is a good fit for a default renderer for Authboss which means it has to deal with text AND html templates (and not just treat text as HTML because that's incorrect).

Can you reevaluate adding a docs site to the repo?

The fact that it's all client-side generated and can be done with a CDN'd index.html makes this more acceptable. My reservations are now limited to two things:

ibraheemdev commented 4 years ago

Ok. I can improve and port over the existing documentation. We can also keep the readme, but link to the docs site at the top as a header. Does that address your concerns?

aarondl commented 4 years ago

Sure. Sounds good. Perhaps there's a badge we could add for the docs site as well.

ibraheemdev commented 4 years ago

Ok I think that's it for this issue. I just have one last question. Considering probably 90% of users will be using gorilla secure cookie, why do you have reservations with adding that as a dependency.

aarondl commented 4 years ago

We shouldn't deal in made up statistics :)

Even if it were true that 90% use gorilla secure cookie, it's also true that the 10% would be hurt by this additional dependency. It's a slippery slope argument.

Also as someone who has done a lot of open source license scanning and reporting for commercial products these sorts of red herring dependencies are quite obnoxious.

But the real question we should be asking is: What actual tangible benefit is there to having fewer modules and combining these other pieces into Authboss directly? People still have to find them somehow somewhere in the docs and import those packages despite them being under the same repository. I think we gain a tiny bit of visibility as the only benefit.

ibraheemdev commented 4 years ago

Ok, I see that you have a set way that you want to go with this project. I'll try to help out and create pr's while respecting you concerns. Thanks for your time 👍