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

Converted to TypeScript and added setup instructions in the README #71

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rushabhhere
Copy link

@rushabhhere rushabhhere commented Jul 31, 2022

Closes #56

@rushabhhere rushabhhere mentioned this pull request Jul 31, 2022
@Tobbe
Copy link
Member

Tobbe commented Jul 31, 2022

Thank you so much @rushabhhere, this is a lot of work!

🚀

README.md Outdated Show resolved Hide resolved
@Tobbe
Copy link
Member

Tobbe commented Jul 31, 2022

@thedavidprice Is this what you had in mind?

@cannikin Can you please look this over? Ignore all the TS stuff, you don't have to look at that 😁 Focus more on the text updates and the general changes made to files. Is it what you'd expect?

@adriatic
Copy link

@Tobbe , @thedavidprice - a few days ago, I reported this Typescript related issue, discovered in the context of trying to create a TypeScript version of the Redwood Tutorial. At that time I was not aware of the same effort by @rushabhhere.

Only today, I found that a different project than Redwood Tutorial exhibits the same (or similar) issue, indicating that the fix should not be possible within Redwood Tutorial code.

To ensure better readability, my issue is defined here as TypeScript problem in graphql.d.ts.

@rushabhhere
Copy link
Author

@adriatic You're right, I faced this issue as well. For the time being, and in the code I am trying to merge with this pull request, I dug into the types/graphql.d.ts file for the correct types and have used only those types that exist and are valid.

@adriatic
Copy link

adriatic commented Jul 31, 2022

@rushabhhere in your paragraph above you stated:

@adriatic You're right, I faced this issue as well. For the time being, and in the code I am trying to merge with this pull request, I dug into the types/graphql.d.ts file for the correct types and have used only those types that exist and are valid.

Besides making a great effort to create the TypeScript Redwood Tutorial application 👏 👏 , you also nailed the TypeScript problem in graphql.d.ts almost at the same time I did. Since I reported my "discovery" to the core team in the issue The type ArticlesQuery does not exist in types/grapql, we have to remember to address your fix in the Redwood Tutorial sample, that allows the building of
that sample.

Since the problem with graphql.d.ts is a missing definition of Prisma, which causes the two lines below to be invalid

  JSON: Prisma.JsonValue;
  JSONObject: Prisma.JsonObject;

causing:

currentUser?: Maybe<Scalars['JSON']>;

Until our smart folks on the core team resolve this issue I am simply using the following replacements:

 JSON: Object;
  JSONObject: Object;

@cannikin
Copy link
Member

Can you please look this over? Ignore all the TS stuff, you don't have to look at that 😁 Focus more on the text updates and the general changes made to files. Is it what you'd expect?

Oh geez I don't know, you'd have to go through the first half of the tutorial, building everything in order, and see if what you end up with matches the code in this repo. Has anyone tried that? When I built this original repo I created it by following the steps of the tutorial just like someone off the street would. Then I added a minimal amount of extra markup to be able to style things a little nicer, and flesh out some of the tests. The second half of the tutorial references code from the first half, and if it doesn't match it's going to be confusing.

Now there's the added complexity of making sure that after converting from TS to JS that the code still matches what you'd get if you went through the JS version from the beginning.

@thedavidprice Are you worried at all about making TS the default for this repo? It's kind of making me nervous. I thought I heard someone say that we're getting more help requests/confusion in Discord since the tutorial added TS code examples. Maybe that was only because of the type generation errors that we've since cleared up?

A TS person will have no problem understanding a JS repo, but the inverse is not true. Yes, you can convert to JS, but I worry about how close that ends up being to the original "hand crafted" version that was JS from the start.

web/src/components/Post/PostForm/PostForm.tsx Show resolved Hide resolved
web/src/pages/ContactPage/ContactPage.tsx Outdated Show resolved Hide resolved
web/src/pages/ContactPage/ContactPage.tsx Outdated Show resolved Hide resolved
web/src/pages/ContactPage/ContactPage.tsx Outdated Show resolved Hide resolved
@cannikin
Copy link
Member

cannikin commented Aug 1, 2022

I saw several function name changes (maybe just personal preference?) like onSubmit -> handleSubmit, and there could be a bunch more that I didn't catch, but I'd change these back. The tutorial itself still has the original names and it would be very confusing to have this codebase not match, when it was supposed to have been built by going through the first half.

If we do decide to change them, we need to update the tutorial text and code snippets to match.

I also saw several examples of custom classNames being removed...did that not change the UI significantly, or were they maybe redundant after getting the latest scaffold.css styles? We haven't refreshed this repo in a while so maybe they were old...

Do you have any screenshots of what this app looks like when you start it up after checking it out from scratch? How does it compare to the current repo?

Does the test suite still pass 100% out of the box?

Do all the Storybook views still work?

@rushabhhere
Copy link
Author

rushabhhere commented Aug 2, 2022

@cannikin My process of making this TypeScript version was exactly as you've described - I went through the first half of the tutorial closely and then compared the styles and tests that were added in this intermission repo and added them to my TS code.

Honestly, it has been a while since I worked on this, so I am not sure about the name changes. I think it might've been personal preference but I honestly don't remember actively changing variable names - there might be only a few of them and we can change them as we spot more. That's a valid concern.

Converting from TS to JS is also a valid concern. There's a possibility it might not match the JS code achieved going through the tutorial using JS. The solution to this can be to make a separate repo to clone for TS users altogether, which can also be maintained by the renovate bot. What approach to take is really for the core team to decide, along with the devs who worked on the TS to JS functionality. They could confirm what kind of JS code the feature generates.

I am attaching some screenshots to help you verify that the app looks exactly like how the app from the current intermission repo looks.

home
article
contact

Yes, all the test suites pass out of the box.

tests

And yes, I have ensured that all the Storybook views work.

Storybook

@Tobbe
Copy link
Member

Tobbe commented Aug 3, 2022

@rushabhhere

Converting from TS to JS is also a valid concern. There's a possibility it might not match the JS code achieved going through the tutorial using JS. The solution to this can be to make a separate repo to clone for TS users altogether, which can also be maintained by the renovate bot. What approach to take is really for the core team to decide

We specifically did not want to have to do this, because we were afraid it was going to be too much work keeping everything updated and in sync

along with the devs who worked on the TS to JS functionality. They could confirm what kind of JS code the feature generates.

We use https://babel.dev/docs/en/babel-plugin-transform-typescript plus a pass through prettier for this feature.

I guess one could take the code in this PR, convert it with ts-to-js, and then make another PR against this repo to see what has changed.

@cannikin

Now there's the added complexity of making sure that after converting from TS to JS that the code still matches what you'd get if you went through the JS version from the beginning.

What are you most concerned would not match anymore? Functionality or just the way the code looks? Function names or stuff like indentation?

A TS person will have no problem understanding a JS repo, but the inverse is not true.

This is true. But if you went through the whole tutorial in JS mode, then started your own project in TS you'd have no idea about all the types that we generate or how to work with those. It's not just about understanding the code, it's also about working efficiently with it

@cannikin
Copy link
Member

cannikin commented Aug 3, 2022

@rushabhhere Thanks for the response and the screenshots! It's possible that the generators themselves have changed and some variable/function name changes were introduced there, but that the tutorial wasn't updated to match. But some like handleSubmit isn't found in the Redwood codebase at all, so I can only assume that was written while going through the tutorial! :)

@Tobbe

What are you most concerned would not match anymore? Functionality or just the way the code looks? Function names or stuff like indentation?

I'm sure it'll function the same, I'm worried about line numbers, line breaks, stuff like that. If the tutorial shows you need to add/change code on lines 13-17 and now those lines are 15-19 that's not ideal. Are the lines different because I did something wrong earlier in the tutorial? There could be discrepancies even now, but I definitely don't want to make them worse.

But if you went through the whole tutorial in JS mode, then started your own project in TS you'd have no idea about all the types that we generate or how to work with those.

We can't optimize for everyone, but my concern is always going to be geared towards more junior folks, those for whom this is maybe even their first framework. If they want to use TS they have the option of going through the whole tutorial in TS, we can't also be worried about people that want to switch midway through.

Right now the tutorial is 100% focused on teaching Redwood fundamentals, maybe there should just be a separate tutorial just geared towards TS folks: less focus on "what is a cell?" and "what is a service?" and more on Prisma types vs. GraphQL types vs. Service types, when to use which where. (It would also give us the opportunity to show building something other than a blog.) As it stands in the tutorial the TS stuff is just randomly introduced in the code snippets but could probably use a much more in-depth discussion. I don't know that we even really have a docs page that summarizes everything...there's so many questions in the forums and Discord, it's definitely something we should have. @dac09 has been going through the docs and adding callouts about TS gotchas where appropriate, but we don't have a single reference for everything TS.

This is your chance to have a blank slate to show the world the power of TS, to mold their psyche to your whim! Without all of my pesky concerns about stuff like "beginning developers" and "making the code easy to read!" And you wouldn't have to constantly deal with my annoying comments and attempted removal of any reference to TS in everything we do! In fact, I can guarantee I will never question or comment about anything anyone writes in that tutorial, ever. ;)

@Tobbe
Copy link
Member

Tobbe commented Aug 4, 2022

I'm sure it'll function the same, I'm worried about line numbers, line breaks, stuff like that. If the tutorial shows you need to add/change code on lines 13-17 and now those lines are 15-19 that's not ideal. Are the lines different because I did something wrong earlier in the tutorial? There could be discrepancies even now, but I definitely don't want to make them worse.

Since we use prettier I'm not too worried this is going to be a problem. But assuming the "ts-to-js"-version of this code looks alright, and there happens to be differences between line numbers and/or the way the code looks that's supposed to match up with code snippets, can't we just update the tutorial to match the "ts-to-js"-version of the code instead of your hand crafted version?

Tobbe: But if you went through the whole tutorial in JS mode, then started your own project in TS you'd have no idea about all the types that we generate or how to work with those.

Rob: we can't also be worried about people that want to switch midway through.

I'm not sure if you misunderstood me, or if I'm misunderstanding you now. But when I said "started your own project in TS", I meant their own first real project after the tutorial. After what was earlier both part 1 and 2 of the tutorial. Not switching in the middle of the tutorial :)

I don't know that we even really have a docs page that summarizes everything

Danny has been doing a great job fleshing out our TS docs https://redwoodjs.com/docs/canary/typescript/introduction

@cannikin
Copy link
Member

cannikin commented Aug 4, 2022

Can't we just update the tutorial to match the "ts-to-js"-version of the code instead of your hand crafted version?

Absolutely! But I think this PR should do that if it turns out that it's needed (maybe it won't be, and the converted JS version already matches).

I'm not sure if you misunderstood me, or if I'm misunderstanding you now. But when I said "started your own project in TS", I meant their own first real project after the tutorial. After what was earlier both part 1 and 2 of the tutorial. Not switching in the middle of the tutorial :)

Yeah sorry, I didn't mean in the middle of the tutorial itself, more like in the middle of your "learning Redwood" journey. hah Like you start doing JS with the tutorial, then decide you want to start an actual app in TS, like you said. I don't know that the tutorial must cater to that type of journey, but it would be nice if it did!

I didn't mean to imply that this PR shouldn't be merged—if you really want to do the tutorial in TS, and we have the code snippets in TS during the tutorial, you should be able to check out that suggested repo in TS as well, I agree. I just can't let the codebase moving to TS degrade the experience for people that want the JS version. 🙂 So as long as the resulting JS code matches the code snippets in the JS version (might require updates to the tutorial JS code snippets), and we address the instances I found of function/variable names being different, I'm all good!

@Tobbe
Copy link
Member

Tobbe commented Aug 4, 2022

Great Rob! Then I feel we have a way forward here 🙂 @rushabhhere are you up for doing the final work too, or do you want one of us to take it over?

@rushabhhere
Copy link
Author

@cannikin, I have resolved the name change issues you pointed out.

@Tobbe, I would be happy to lend a hand in doing the final work too! I suppose this primarily involves making changes to code snippets in the tutorial following the intermission? Just let me know how I can help, and I will.

@rushabhhere
Copy link
Author

I guess one could take the code in this PR, convert it with ts-to-js, and then make another PR against this repo to see what has changed.

This makes sense. I can't do this because I have already forked this repository and you cannot fork a repository more than once on GitHub. If someone else could do this, it would be a tremendous help.

@Tobbe
Copy link
Member

Tobbe commented Aug 6, 2022

I can't do this because I have already forked this repository

Just create another branch in your existing fork and make a PR from that branch

@rushabhhere rushabhhere mentioned this pull request Aug 7, 2022
@rushabhhere
Copy link
Author

Just create another branch in your existing fork and make a PR from that branch

Oh yes, what was I thinking! Here: #73.

@Tobbe
Copy link
Member

Tobbe commented Aug 7, 2022

I just skimmed it, but looks reasonable I think. @cannikin is on vacation. Let's ping him in 10 days or something and we'll try to get his input again. Until then it would be awesome to see what kinds of JS updates to the tutorial are needed

@rushabhhere
Copy link
Author

Until then it would be awesome to see what kinds of JS updates to the tutorial s needed

Didn't get you.

@Tobbe
Copy link
Member

Tobbe commented Aug 8, 2022

@rushabhhere Sorry. Let me try again :)

For the second half of the tutorial we assume the user is using this repo as a base for when going through the tutorial.
So any JS (and TS) code we show in the tutorial needs to match what's in this repo. So for JS it needs to match whatever the user gets after running ts-to-js.

@Tobbe
Copy link
Member

Tobbe commented Oct 2, 2022

@rushabhhere Really sorry we dropped the ball on this one 🙁 Seems like it's fallen behind a bit with the tutorial and general updates to RW itself.

Are you up for trying to bring this PR up to date again? Or do you want me to do it?

@rushabhhere
Copy link
Author

@Tobbe I am up for the task, just give me a week's time!

@rushabhhere
Copy link
Author

@Tobbe I am sorry for the delay. I am working on this and I think I should be done by Sunday (16th October) at max.

@Tobbe
Copy link
Member

Tobbe commented Oct 13, 2022

@rushabhhere Awesome! Thanks for the update 🙂

@rushabhhere rushabhhere reopened this Oct 16, 2022
@rushabhhere
Copy link
Author

rushabhhere commented Oct 16, 2022

@Tobbe I messed up a bit while trying to update the PR, but I think it's good now. I have bumped it up to v3.2.0, please check if this is what's needed.

@cannikin
Copy link
Member

cannikin commented Nov 7, 2022

There are a couple of conflicts here, do you think you can take a look at resolving them?

@cravend
Copy link

cravend commented Mar 14, 2023

Hi all, I just finished the first half of the tutorial, started to clone this repo for section 2, and then saw the issues about TypeScript. Given that this PR has gone stale, what are thoughts on taking the part one repo (which I have with almost no changes from the tutorial): https://github.com/cravend/redwoodblog, and adding to it the storyboard components / styles / etc. I think that may be easier than trying to refactor this repo to use TS.

@thedavidprice
Copy link
Contributor

Hi @cravend Thank you for adding momentum here. I'm looping in @jtoar for visibility (so we don't lose this conversation).

Could you be more clear about what you're suggesting, e.g. what's the end goal? (I'm assuming you're trying to create a TS version of this repo...)

Regardless, I completely agree this PR has gone stale. You'd be welcome to open a new one, just reference here if so.

This project would be a lot of work! We'd welcome the help but just want to make sure you're aware. (Note: sometimes with projects like this it's better to work piecemeal against a branch — create a primary TS conversion branch and then work chapter by chapter against it. That can be easier on us for review as well. TBD)

@adriatic
Copy link

This is a long thread that indicates the difficulties of finding the proper solution. I think that @cannikin stated that best:

A TS person will have no problem understanding a JS repo, but the inverse is not true. Yes, you can convert to JS, but I worry about how close that ends up being to the original "hand crafted" version that was JS from the start.

While I did translate my own copy of the tutorial to TypeScript (once) I would hate to be tasked to redo this for every (Javascript) change of the tutorial. The alternative (freeze JS tutorial and continue evolving its TS version) would be very unfriendly to all JS folks, so it should not be considered.

Lastly automatic "JS to TS" translation is hardly feasible as explained here.

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.

Typescript support
6 participants