-
Notifications
You must be signed in to change notification settings - Fork 109
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
base: main
Are you sure you want to change the base?
Conversation
Thank you so much @rushabhhere, this is a lot of work! 🚀 |
@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? |
@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. |
@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 |
@rushabhhere in your paragraph above you stated:
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 Since the problem with
causing:
Until our smart folks on the core team resolve this issue I am simply using the following replacements:
|
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. |
I saw several function name changes (maybe just personal preference?) like 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 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? |
@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. Yes, all the test suites pass out of the box. And yes, I have ensured that all the Storybook views work. |
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
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
What are you most concerned would not match anymore? Functionality or just the way the code looks? Function names or stuff like indentation?
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 |
@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
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.
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. ;) |
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?
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 :)
Danny has been doing a great job fleshing out our TS docs https://redwoodjs.com/docs/canary/typescript/introduction |
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).
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! |
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? |
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. |
Just create another branch in your existing fork and make a PR from that branch |
Oh yes, what was I thinking! Here: #73. |
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 |
Didn't get you. |
@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. |
@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? |
@Tobbe I am up for the task, just give me a week's time! |
@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. |
@rushabhhere Awesome! Thanks for the update 🙂 |
@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. |
There are a couple of conflicts here, do you think you can take a look at resolving them? |
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. |
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) |
This is a long thread that indicates the difficulties of finding the proper solution. I think that @cannikin stated that best:
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. |
Closes #56