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

add/typescript files #175

Closed
wants to merge 1 commit into from
Closed

add/typescript files #175

wants to merge 1 commit into from

Conversation

vmcodes
Copy link
Collaborator

@vmcodes vmcodes commented Jun 14, 2023

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.

@vmcodes vmcodes requested a review from cherylli June 14, 2023 04:43
@netlify
Copy link

netlify bot commented Jun 14, 2023

Deploy Preview for priceless-booth-2dfcaf failed.

Name Link
🔨 Latest commit 3ccd657
🔍 Latest deploy log https://app.netlify.com/sites/priceless-booth-2dfcaf/deploys/6489455ee7b22800080f9514

@vmcodes vmcodes requested a review from mariana-caldas June 14, 2023 04:43
@vmcodes vmcodes self-assigned this Jun 14, 2023
@vmcodes
Copy link
Collaborator Author

vmcodes commented Jun 14, 2023

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.

Copy link
Member

@cherylli cherylli left a comment

Choose a reason for hiding this comment

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

There are build errors. Don't think you can convert to ts just by changing the filenames. It won't build, you have to add types.

It might work locally for dev but it won't build for production.

@cherylli
Copy link
Member

cherylli commented Jun 14, 2023

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
Copy link
Collaborator Author

vmcodes commented Jun 14, 2023

@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
Copy link
Member

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
Copy link
Collaborator Author

vmcodes commented Jun 14, 2023

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
Copy link
Member

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
Copy link
Collaborator Author

vmcodes commented Jun 14, 2023

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
Copy link
Member

mariana-caldas commented Jun 14, 2023

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!

@vmcodes vmcodes closed this Jun 16, 2023
@vmcodes vmcodes deleted the chore/add-typescript branch June 16, 2023 00:52
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.

3 participants