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

[Bug?]: dbAuth userAttributes comment error #10456

Closed
pantheredeye opened this issue Apr 14, 2024 · 7 comments
Closed

[Bug?]: dbAuth userAttributes comment error #10456

pantheredeye opened this issue Apr 14, 2024 · 7 comments
Labels
bug/needs-info More information is needed for reproduction topic/auth

Comments

@pantheredeye
Copy link
Collaborator

What's not working?

After setting up dbAuth, the signupOptions code in api\src\functions\auth.js has an example user attribute that is commented out. Well, I needed that exact field, so I uncommented it and ran into an error on the next signup attempt. I ended up adding an underscore to match the props in the handler, changing it from name: userAttributes.name to name: _userAttributes.name and sign up worked.

I am not sure what the _ indicates, but it is in the params for the handler, so I am guessing it should be included the commented out line?

This search: https://github.com/search?q=repo%3Aredwoodjs%2Fredwood%20userAttributes.name&type=code has quite a few files show up... if its a matter of going through most of these and adding _, could we label this "Good first Issue?"

Generated Code:

    handler: ({
      username,
      hashedPassword,
      salt,
      userAttributes: _userAttributes,
    }) => {
      return db.user.create({
        data: {
          email: username,
          hashedPassword: hashedPassword,
          salt: salt,
          // name: userAttributes.name
        },
      })
    },

Working code with accompanying paragraph at the top:

  const signupOptions = {
    // Whatever you want to happen to your data on new user signup. Redwood will
    // check for duplicate usernames before calling this handler. At a minimum
    // you need to save the `username`, `hashedPassword` and `salt` to your
    // user table. `userAttributes` contains any additional object members that
    // were included in the object given to the `signUp()` function you got
    // from `useAuth()`.
    //
    // If you want the user to be immediately logged in, return the user that
    // was created.
    //
    // If this handler throws an error, it will be returned by the `signUp()`
    // function in the form of: `{ error: 'Error message' }`.
    //
    // If this returns anything else, it will be returned by the
    // `signUp()` function in the form of: `{ message: 'String here' }`.
    handler: ({
      username,
      hashedPassword,
      salt,
      userAttributes: _userAttributes,
    }) => {
      return db.user.create({
        data: {
          email: username,
          hashedPassword: hashedPassword,
          salt: salt,
          name: _userAttributes.name
        },
      })
    },
    ```

### How do we reproduce the bug?

_No response_

### What's your environment? (If it applies)

_No response_

### Are you interested in working on this?

- [ ] I'm interested in working on this
@pantheredeye pantheredeye added the bug/needs-info More information is needed for reproduction label Apr 14, 2024
@ahaywood
Copy link
Contributor

@pantheredeye I'm meeting with @dthyresson later today, I'll ask him about the _

@dthyresson
Copy link
Contributor

dthyresson commented Apr 17, 2024

@pantheredeye The _ means that the variable or argument is unused. It's needed as linting and TS will warn that you have declared userAttributes but not used it anywhere in the function. It's to prevent extraneous or ... well .. unused variables.

The templates sets it as _userAttributes so one knows it is available -- but since // name: userAttributes.name does not in fact use it TS and lining won't complain.

In you case, just change the argument to be without the underscore and then in the user create data refer to the variable as name: userAttributes.name so everything matches up.

@pantheredeye
Copy link
Collaborator Author

pantheredeye commented Apr 17, 2024

That all makes sense and is easy enough. I was really hoping that we could make a note of sorts to inform the user how to handle this by either adding the _ or removing the _. Being naive to this, I kept the _ because I did not want to conflict with any existing userAttributes somehow.
🤷‍♂️

@pantheredeye
Copy link
Collaborator Author

How do we close this? I do feel like this will trip someone else up, so I would like to have a note in the docs explaining this. If that is a no vote, then we can just close this issue, correct?

@dthyresson
Copy link
Contributor

If anything I’d update the template to:

  1. add comment note to be sure to correct the unused variable
  2. Or add a typescript ignore to allow unused variables and remove the underscore

I do think the ide and typescript give rather informative messaging to let the developer implement correctly

@Tobbe
Copy link
Member

Tobbe commented May 14, 2024

I think that in many cases we already have too many and too long comments in the generated code. At some point people will just not read it because it's too much.

I also don't like the idea of disabling the linting rule. You really shouldn't have unused variables littering your code.

And we also don't want to generate code with red squiggles.

It's a difficult balance 🙂

As @pantheredeye found out, using underscores, like in _unusedVariable, is something we do in a lot of places. It's a general pattern that probably warrants a general note in our docs. Maybe we should have a section about all the default linting rules we ship with. Ideally with a description of what they do and also a motivation as to why we've chosen to include them.

I don't think we should add a comment in the code at every place we use the underscore for unused variables.

@pantheredeye
Copy link
Collaborator Author

Tobbe's suggestion seems fine. Although, I question who would read a document like that before running into an error like this. It would be great if we could somehow introduce the message into the IDE error messaging.

At this point it feels like since no one has brought this up before, most people probably understand it. But, from my perspective, as a dev who is not confident in the intricacies of the framework, it is scary when I run into an "error" inside of an auth package while essentially following the instructions and uncommenting // name: userAttributes.name. If the _ is elsewhere and I bumped into it, I would still be worried even if it wasn't in auth.

image

I actually googled the underscore before making the issue to see if I could figure it out. Even after my search, I did not understand that the underscore indicated an unused value. I was afraid that removing the underscore might make my code interfere with framework code somewhere else.

The "ide and typescript give rather informative messaging to let the developer implement correctly". The IDE's messaging was Did you mean '_userAttributes'? and '_userAttributes' is declared here. This is informative. But for me, not knowing any better, I followed the suggestions and changed it to name: _userAttributes.name. This is not how you would suggest fixing it. It isn't how I would suggest fixing it, now that I know better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/needs-info More information is needed for reproduction topic/auth
Projects
None yet
Development

No branches or pull requests

4 participants