processing / p5.js-web-editor

The p5.js Editor is a website for creating p5.js sketches, with a focus on making coding accessible and inclusive for artists, designers, educators, beginners, and anyone else! You can create, share, or remix p5.js sketches without needing to download or configure anything.
https://editor.p5js.org
GNU Lesser General Public License v2.1
1.3k stars 1.27k forks source link

Basic Config for `typescript` and changed user model to test the system. #3015

Closed Swarnendu0123 closed 1 month ago

Swarnendu0123 commented 4 months ago

Ref #3004

Changes: Hello @lindapaiste as you mentioned, I have created the Basic configurations for typescript and changed the server\models\user.js to server\models\user.ts to test the system. But still I am getting some error in server\models\user.ts

const callback = typeof options === 'function' ? options : cb;
  if (isEmail) {
    return this.findByEmail(value, callback);
  }
  return this.findByUsername(value, callback);
};

In the above snipped, its looks like there are some problem in the definition of findByEmail and findByUsername.

userSchema.statics.EmailConfirmation = EmailConfirmationStates;

in the above code, the userSchema.statics.EmailConfirmation is throwing some error. can you please suggest how to came out this error?

lindapaiste commented 4 months ago

My recollection of working with Mongoose and TypeScript is that it's not as automatic as you would hope. We probably need to annotate our extra methods somewhere. I'll take a closer look later. Also there are some packages for using Mongoose + TS together which may or may not be worth using. If the main goal of this PR is to establish a configuration, then maybe we choose a simpler test file. Like something in /server/utils.

lindapaiste commented 4 months ago

Look at that, we found an actual error! Thank you TypeScript! image

I've got to look into where we are using that findByEmailOrUsername function and why we haven't noticed that problem before.


I got it (mostly) working but it's extremely repetitive. There's got to be a better way.

type FindUserCallback = (err: Error | null, doc?: User) => void;

interface UserStatics {
  findByEmail: (
    email: string | string[],
    cb?: FindUserCallback
  ) => void;
  findByUsername: (
    username: string,
    options?: { caseInsensitive?: boolean },
    cb?: FindUserCallback
  ) => void;
  findByEmailOrUsername: (
    value: string,
    options?: { caseInsensitive?: boolean; valueType?: 'email' | 'username' },
    cb?: FindUserCallback
  ) => void;
  findByEmailAndUsername: (
    email: string,
    username: string,
    cb?: FindUserCallback
  ) => void;
  EmailConfirmation: typeof EmailConfirmationStates;
}

interface UserModel extends Model<User>, UserStatics {
  statics: UserStatics;
}

const userSchema = new Schema<User, UserModel>(

Then use UserModel where you have Model<User>.

Swarnendu0123 commented 4 months ago

@lindapaiste I tried. but unfortunately, I am getting a lots of error. Can you please go through this PR, and and push a commit over this PR?

lindapaiste commented 4 months ago

Looks like we need to make some changes to the Jest setup so that Jest can read the .ts file.

Swarnendu0123 commented 4 months ago

But when I tried to set up the configurations, the main error was some eslint version error, maybe that is the root of all of those errors.

I'm quite proficient in writing tests using Jest or Enzyme, both of which we already use in our current repository. I can assist in creating additional test files and enhancing them by incorporating type safety.

Let me give it a try, again.

lindapaiste commented 4 months ago

But when I tried to set up the configurations, the main error was some eslint version error, maybe that is the root of all of those errors.

Yes omg I have had an open PR to fix that for ages. Every time you install a package you have to use --legacy-peer-deps or else it won't work. If that's the issue. Maybe not. I was getting some sort of error with using the "@typescript-eslint" eslint plugin. So I removed that but then I was getting errors about missing file extensions from the "import" plugin. I can try to help out but I'm not sure when I'll have time to take a deeper look.

Swarnendu0123 commented 4 months ago

Every time you install a package you have to use --legacy-peer-deps or else it won't work. If that's the issue.

🥲 Unfortunately yes! Additionally "can't use import statement outside of a module"

lindapaiste commented 4 months ago

BTW the other TypeScript error that I was seeing has to do with setting userSchema.statics.EmailConfirmation = EmailConfirmationStates; as the types for statics indicate that is should be used for functions, and we are using it to store a dictionary object. TBH I'm more of a frontend person than a backend person so I'm not sure if there's a better or different place to store those values. Possibly that's an error that we would be okay with ignoring, at least in the short term.

Swarnendu0123 commented 4 months ago

BTW the other TypeScript error that I was seeing has to do with setting userSchema.statics.EmailConfirmation = EmailConfirmationStates; as the types for statics indicate that is should be used for functions,

Yes, I have already mentioned that in the PR description.

and we are using it to store a dictionary object. TBH I'm more of a frontend person than a backend person so I'm not sure if there's a better or different place to store those values. Possibly that's an error that we would be okay with ignoring, at least in the short term.

Let me do some sort of research on it. Most probably I will come up with a solution.

Maybe not. I was getting some sort of error with using the "@typescript-eslint" eslint plugin. So I removed that but then I was getting errors about missing file extensions from the "import" plugin.

But I need your help in this.

lindapaiste commented 4 months ago

FYI there's also some trickiness with the Mongoose exec() function because their types declare that it returns void if a callback is provided or a Promise if there is no callback.

This is what's in the types for mongoose version "5.13.20" which we are using:

    /** Executes the query */
    exec(): Promise<ResultType>;
    exec(callback?: Callback<ResultType>): void;
    // @todo: this doesn't seem right
    exec(callback?: Callback<ResultType>): Promise<ResultType> | any;

In the long term we can avoid that issue entirely if we modernize the codebase and stop using callbacks, which are largely an outdated legacy syntax. But I don't know if that's a priority to @raclim. The current version of mongoose does not even allow passing a callback to exec(). So the type is a lot simpler:

    /** Executes the query */
    exec(): Promise<ResultType>;

https://github.com/Automattic/mongoose/blob/1f340d4e3a846abe520beb9b2f659b2eb3fe1a35/types/query.d.ts#L213-L214

lindapaiste commented 4 months ago

I fixed some of the issues and pushed a commit. ✅

Swarnendu0123 commented 4 months ago

Still I am getting some error, is it running for you? If yes, probably, I have to setup the branch again in my lical system. And we can start working on it. In my opinion, the frontend part will be easy to convert,

can I convert any small component for testing purposes?

raclim commented 1 month ago

Thanks for your work on this! After giving it some thought I don't think implementing TypeScript will be on the roadmap in the near future in favor of addressing some of the other technical debt in this repository first. Once that's done though, I do think this can be revisited around later in the year or early next year. I'm going to add this to a Board for Saved PRs to Revisit to look at until we're ready to come back to this!