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

OsoClass decorator unclear #432

Open
weshuiz opened this issue May 11, 2023 · 5 comments
Open

OsoClass decorator unclear #432

weshuiz opened this issue May 11, 2023 · 5 comments

Comments

@weshuiz
Copy link

weshuiz commented May 11, 2023

The documentation for the @OsoClass decorator could be explained in more detail. Firstly, how is this decorator used? Nest and Oso need to be aware of its existence, and I assume that classes using the decorator need to be imported. Through trial and error, I figured out that it needs to be provided in the app.module, but nothing was mentioned about this, and I'm not even sure if this is the correct method.

Secondly, registered Oso classes need to have actual values set, which isn't done correctly either. Instead, we get this:

import { OsoClass } from 'nestjs-oso';

@OsoClass()
export class User {
  id: string;
}

Thirdly, the @OsoClass decorator does not allow constructors with a value argument in them, which could have been mentioned, or at least given as a heads up.

Lastly, wouldn't it be nice if a guard were provided instead of expecting us to always write it ourselves?

Sincerely, someone who spent months figuring out these flaws on their own because the documentation was non-existent.

PS: No wonder people prefer Stack Overflow over using official docs (clasic meme)

@simenandre
Copy link
Member

Thanks for posting this!

Let's try and update the documentation on this. Want to take a stab at it?

@weshuiz
Copy link
Author

weshuiz commented May 11, 2023

I would love to help updating this.
But how would i help updating the docs for something i struggle to understand my self?
but thanks to my struggles i think i can make a small summing up

First: i think we can start by mentioning registered classes would need to imported in the app.module as provider
second: people are likely to create a seperate file like model or entity.ts
a file like this does not have access to things like the Request object so a example would be needed to show the classes get access the database records so a actuel value can be set/asigned
Third: the decorator does not seem to accept constructors with a variable defined (even if unused)
this would either needed to be fixed so things like the req object can be passed or for people with passport req.user
or some kind of warning in the docs telling people this is a limitation

@weshuiz
Copy link
Author

weshuiz commented May 15, 2023

i added some basic suggestions how the docs could be improved reguarding this issue
but i'm afraid this is all i can do (as i prob implented this wrong as well my self)

@edgahan
Copy link

edgahan commented Jul 3, 2023

I have started researching how I would use oso with Nest and came across https://github.com/osohq/oso-nest-doc-mgmt - this might be helpful to others as a reference.

@simenandre
Copy link
Member

simenandre commented Jul 3, 2023

Would it make sense to create an example like the one @edgahan referenced? By the way, oso-nest-doc-mgmt is not based on nestjs-oso, which might confuse folks if we refer to that. There might be an argument that we should update to use nestjs-oso in that repository to build a community around NestJS/OSO users here.

@weshuiz, I would love to see a repository where you have implemented nestjs-oso; it's easier to give pointers, and that repository might also serve as an example! 🙌

Documentation is complicated, but I think we're better served if someone who finds them lacking updates it.

Thanks for helping out, @weshuiz and @edgahan! ☀️

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

3 participants