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

Fix typescript errors #9

Closed
wants to merge 6 commits into from

Conversation

KrisCoulson
Copy link
Contributor

yarn rw storybook fails to compile if there are typescript errors.

This is a WIP I fixed quite a few of the typescripts errors

I was having a little trouble running down why some were throwing.

@noire-munich would you be able to pick up from here and clean these last few typescript errors up.
I also think there are a few tests that are failing at the moment from database schemas that have changed

@KrisCoulson
Copy link
Contributor Author

@realStandal @cannikin do you guys mind taking a look at this / merging? Lol I don't want it to get any bigger 🤣

@cannikin
Copy link
Member

cannikin commented Mar 6, 2022

Sorry man, I hate Typescript with every fiber of my being and specifically didn't start the project as a TS one. I wouldn't know if the changes in there are correct or not! I'm also not worried about Storybook or tests at this point, I'm just trying to get everything live in time for the 1.0 launch. I figure we can circle back to those when we've got time. At least, that's my plan for job board—if others think they've got the time for the other sections of the site, that's great!

Should I just rename my files to .js and .jsx so I'm not creating new errors, or does that break everything? Once you enable TS for the project are you stuck with it everywhere, forever?

@KrisCoulson KrisCoulson force-pushed the fix-typescript-errors branch from 657b211 to acb75bc Compare March 6, 2022 17:42
@KrisCoulson
Copy link
Contributor Author

KrisCoulson commented Mar 6, 2022

@cannikin I actually figured out a way to turn off typescript in storybook in #10 so it won't fail to load if we try to spin it up. So no need to do anything differently.

I mainly just want to use storybook to test a few components and interactions out in isolation and think it would be good if it works.

And no worries. I get it. I haven't used much typescript myself. One thing I notice when making all these fixes that might make it a little less annoying is redwood generates the graphql types for you.

So you don't need to do this in your services

interface CreateJobProfileArgs {
  input: Prisma.JobProfileCreateInput
}

you can just

import { MutationcreateJobProfileArgs } from 'types/graphql'

it just prefixes Query or Mutation and then the name of your service

@realStandal
Copy link
Contributor

TypeScript is... nuanced to say the least (I don't hate it - but I also think you don't need a sledgehammer to hang-up a picture frame). When @noire-munich and I first spoke, we agreed too that JavaScript would be easier/let us move faster. I think the generator is picking up on TypeScript now, so every new page, component, ... is being generated in TypeScript.

@cannikin From my understanding, and for the purpose of argument/in the context of RW, I think JavaScript and TypeScript can co-exist. There may be some issues when importing JS components/files into TS - but that'll be the type server breaking, not it being "wrong"; both sides are setup to support JS alongside TS.

All that to say, if your existing .ts and .tsx files aren't causing the application to fail to build, then I don't see a reason to have to change them over (I'm not factoring in Storybook working/not working). I don't mind correcting any errors I come across. The PR @KrisCoulson just opened should get around them causing Storybook to break anyway.

Anyway, @KrisCoulson, I don't mind checking this PR if you'd like to still merge it. But imo, I think disabling type checking (#10) to get Storybook to work until post-launch is perfectly fine; I noticed it errored out but had the same sentiments as Rob about leaving it be. And then we can call the project "JavaScript" and turn a blind eye to its TypeScript parts until the time is there, or the decision is made to remove any TypeScript 🤷

@KrisCoulson
Copy link
Contributor Author

@realStandal I don't mind forgetting this PR unless we just want to merge it to get it out of the way. Most of these types will probably need to be fixed in the future anyway.

Basically, I just ran yarn rw tsc saw the errors, and fixed them so it would stop yelling and compile correctly.

Since then I figured out how to disable typescript checking in storybook we just need to merge #10 then we don't have to worry about types and can fix them later down the road.

Copy link
Contributor

@realStandal realStandal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This is my first (ever) review of someone else's code, apologies if I did that wrong)

@KrisCoulson now that I got the chance to go over this, I'm very grateful you did it and pushed for it to get merged :)

input UpdateUserInput {
email: String
roles: [Roles]!
}

type Mutation {
createUser(input: CreateUserInput): User! @skipAuth
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this resolver will be used on the admin dashboard, so @requireAuth is needed.

const options = {
orderBy: { createdAt: 'desc' },
}
} as Prisma.JobProfileFindManyArgs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this type can be relocated to become:

const options: Prisma.JobProfileFindManyArgs = { ... }

Which should still satisfy the options.take = ... below

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this is different, to the best of my understanding as is a way to force a type transformation, the correct way to assign a type is indeed this notation: const options: Prisma.JobProfileFindManyArgs = { ... }.
No expert though.

const options = {
orderBy: { createdAt: 'desc' },
}
} as Prisma.JobFindManyArgs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here as jobProfiles().

title: string
media?: Record<string, string>
title?: string
media?: Record<string, string | number>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the looks of it, this prop is to be used to store ones that're spread on an <img> element. I think you can mimic this in the prop's type.

For example:

import type { ComponentPropsWithoutRef as CP } from 'react'

interface CardProps {
  media?: CP<'img'>
  ...
}

Your choice, as I'd say it's a matter preference on specificity.

export const generated = () => {
return <ShowcaseJobCard />
return <ShowcaseJobCard {...props} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, you may represent the default props of a Story as:

import type { Story } from '@storybook/react'

import type { ShowcaseJobCardProps } from './ShowcaseJobCard'

const props = { ... }

export const generated: Story<ShowcaseJobCardProps> = (args) => <ShowcaseJobCard {...args} />
generated.args = props

ShowcaseCardProps is an optional generic, and will type args as well as generated.args.

This has the added benefit of ensuring the component's props can be controlled from within Storybook (iirc, yours may too - please lmk if so :) ).

@KrisCoulson
Copy link
Contributor Author

Closing this as it is incredibly outdated.

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

Successfully merging this pull request may close these issues.

None yet

4 participants