search

Web-Dev-Path / web-dev-path

The Web Dev Path platform. Progressive Web App (PWA). Next.js rules!
https://webdevpath.co
GNU General Public License v3.0
33 stars 11 forks source link

add/typescript files #175

Closed vmcodes closed 1 year ago

vmcodes commented 1 year ago
Web Dev Path
174

Have you updated the CHANGELOG.md file? If not, please do it.

Not yet, was looking for guidance.

What is this change?

Updated file names to be compatible with TypeScript and added TypeScript dependencies.

I think that it may be a good idea to at least change the files names before getting started, so that if any other issues are worked on at the same time, things won't get too messy when merging.

Were there any complications while making this change?

No.

How did you verify this change?

Everything worked locally for me.

When should this be merged?

This should be reviewed first, beginning with a draft.

netlify[bot] commented 1 year ago

Deploy Preview for priceless-booth-2dfcaf failed.

Name Link
Latest commit 3ccd6572efa8f8f5b8443ed495332ad691fecac2
Latest deploy log https://app.netlify.com/sites/priceless-booth-2dfcaf/deploys/6489455ee7b22800080f9514
vmcodes commented 1 year ago

I wanted to draft this PR just to get the discussion started around beginning to at least rename the files, so that other issues can still be worked on. If you all think this is a good idea, I can correct some of the issues seen in the build. Thank you.

cherylli commented 1 year ago

I think we can keep the discussion in the existing thread in the issues tab. I thought you wanted reviews to merge into main since this is a pull request.

vmcodes commented 1 year ago

@cherylli I did want to merge this to avoid issues later. I just wasn't sure if that would be a good approach. I kept it as a draft because I planned to fix the build issues if everyone thought it was an okay idea to move forward with. I think some of the errors, like potentially undefined variables are fine and quick fixes. You can keep JS in TypeScript without issues if you add noImplicityAny: false as a temporary compiler option even though the file type extensions are different.

cherylli commented 1 year ago

Yeah I'm not sure. I don't think we should push any temporary thing to production unless it's an urgent bandaid fix.

vmcodes commented 1 year ago

That's not an issue with me at all, do you think then maybe this issue should be completed before 165 since no one has started working on it yet? If so, I can just close this out.

mariana-caldas commented 1 year ago

Hey, @vmcodes !

It seems like a nice start PR! The idea here was just to change the file naming and, then, keep doing the conversion file by file, right? If so, that's a way of doing it, but I have to agree with Cheryl: we don't push into production anything that is not fully ready because it's a risk you don't need to take.

We could create a dev branch to handle this approach. In that case, we would:

  1. Create a dev branch where to keep merging the TS conversion increments. That would allow having more than one developer working with the feature if you get busy or stuck.
  2. When the dev branch is ready - no questions nor build errors - we merge it into main, which will lead to prod.

How do you find this approach?

vmcodes commented 1 year ago

That makes sense to me, I already changed all the file names though on chore/add-typescript, but I can make dev if you prefer. My question would be is once I finish the build issues, but before I add the types, should I make the PR so people can still work coding in JS? I was just thinking that it would avoid merge conflicts.

mariana-caldas commented 1 year ago

That makes sense to me, I already changed all the file names though on chore/add-typescript, but I can make dev if you prefer. My question would be is once I finish the build issues, but before I add the types, should I make the PR so people can still work coding in JS? I was just thinking that it would avoid merge conflicts.

More than just preventing merge conflicts, our priority should be to ensure that unfinished work is not pushed to production, Vincent. Here are the recommended steps:

  1. Change the current branch chore/add-typescript to point to a new branch created from main, specifically the dev branch that already exists (https://github.com/Web-Dev-Path/web-dev-path/branches).

image

  1. Prioritize resolving any build problems before proceeding with further conversions. This may involve undoing some file renaming if necessary.

  2. Once the build problems are solved, you have two options: a. Request a review and merge the chore/add-typescript branch into dev to initiate a new pull request (PR) from the dev branch, such as chore/add-typescript-phase-2. b. Continue working on the current branch without merging anything into dev until the work is completed.

    This approach allows main to remain available for emergency adjustments in simple JavaScript through proper PRs. In case of conflicts between dev and main, resolving them should not be difficult as we handle everything through PRs. In the worst-case scenario, we can simply copy over any modifications.

Please let me know if this explanation is clear, Vincent!