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

Revamping Authboss!!! #305

Closed
6 tasks
ibraheemdev opened this issue Aug 13, 2020 · 9 comments
Closed
6 tasks

Revamping Authboss!!! #305

ibraheemdev opened this issue Aug 13, 2020 · 9 comments

Comments

@ibraheemdev
Copy link
Contributor

ibraheemdev commented Aug 13, 2020

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.

  • I created 3 generators using cobra. Running:
    authboss generate:templates ./destination
    authboss generate:user ./destination
    authboss generate:config ./destination
    Will generate the corresponding file/folder in the directory specified. These are located in internal/generators. These act just like devise generators, and are very useful. This requires moving the templates into the master repo, which I think should be done no matter what. I will add simple bootstrap css as well soon.
  • Improve the docs!!! You can view the beautiful docs site I created with docsify. They are very simple markdown pages, and deployed with github pages in the /docs directory
  • Convert expire to use side effects. This makes the whole project consistent, and was done in this commit
  • Simplify the sample application, make it actually compile, and put it in /examples. I am currently fixing some bugs, but this will be done soon.
  • Create a default html renderer. The abrenderer repo was quite overcomplicated. My implementation is a very simple file and very crucial for new users. This file is well tested, and accepts an optional layout template. It works for the view renderer and mail renderer.
  • Merge the authboss-clientstate into the main repo. This is CRUCIAL. It is only 2 files, and helps make the repo more appealing to users. The two added dependencies are part of the gorilla toolkit and very well maintained.

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

@ibraheemdev ibraheemdev changed the title Completely revamping Authboss! Revamping Authboss!!! Aug 13, 2020
@aarondl
Copy link
Member

aarondl commented Aug 14, 2020

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:

  • Generators: Can we replace these with files people could just copy paste instead of maintaining a binary and bringing in many more dependencies to the repo?
  • Doc site: I don't see this as necessary. They are very pretty docs but the process to get them and the barriers it creates doesn't seem worthwhile. As far as I'm concerned there's nothing wrong with the README - though I'll be the first to admit its contents could be improved.
  • Expire side effects: This change should be reverted as it goes against the current plan for the library.
  • Simplify authboss sample: This change should be reverted as it removes so much functionality. The idea behind the sample is to show off as much as humanly possible in a big "real world" app. This change also brings in additional dependencies and that should be avoided.
  • Create a default html renderer: Text templates must be accommodated as they're a core part of Authboss' design.
  • Merge authboss-clientstate: This change brings in unwanted dependencies into people's projects and must be reverted.

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
Copy link
Contributor Author

ibraheemdev commented Aug 14, 2020

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
Copy link
Contributor Author

ibraheemdev commented Aug 14, 2020

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
Copy link
Member

aarondl commented Aug 14, 2020

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:

  • Sites that interact with README's are now much less useful (for ex pkg.go.dev will show the readme in its entirety)
  • The effort of having to port over all the existing documentation.

@ibraheemdev
Copy link
Contributor Author

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
Copy link
Member

aarondl commented Aug 14, 2020

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

@ibraheemdev
Copy link
Contributor Author

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
Copy link
Member

aarondl commented Aug 14, 2020

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
Copy link
Contributor Author

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 👍

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

No branches or pull requests

2 participants