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

Type checking in the build process #37

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

Conversation

christophstockinger
Copy link

This pull request adds a tsc (TypeScript Compiler) task to the build process. Currently, the build pipeline does not include a TypeScript type-checking step, which means that potentially erroneous TypeScript code could be deployed to production without being caught during the build process.

By incorporating the tsc task, we ensure that type errors are detected early in the build cycle, preventing invalid or mis-typed code from being pushed to production. This enhances the overall stability and reliability of the codebase and reduces the risk of runtime errors caused by type mismatches. Type checking during the build process also helps maintain consistency in our code, making it easier to spot bugs early on and ensuring a higher level of confidence in the production deployment.

Benefits:

  • Early Error Detection: TypeScript's type checking will be enforced during the build, catching errors early before code reaches production.
  • Code Quality Assurance: Including the tsc task ensures that only type-safe code is included in production builds, reducing the likelihood of runtime type errors.
  • Improved Developer Experience: Developers will be alerted to type issues during the build process, helping to maintain cleaner and more maintainable code.

Please review and provide feedback. Thank you!

@christophstockinger
Copy link
Author

christophstockinger commented Feb 24, 2025

⚠️ ⚠️ ⚠️ This PR requires changes to PR #36 for the CI test pipeline to exist, which currently assigns an interface to a type in the useForm hook in the forms, which leads to a build error!

@christophstockinger
Copy link
Author

Since #36 was rejected, I have made the changes regarding type-checking in this PR.

⚠️ In addition, the order of the steps in the Github test action was adjusted, the npm run build requires the composer dependencies due to ziggy-js.

@taylorotwell
Copy link
Member

Does this need to be ported to Vue starter kit as well?

@taylorotwell taylorotwell marked this pull request as draft February 25, 2025 18:15
@taylorotwell taylorotwell marked this pull request as ready for review February 25, 2025 18:21
@christophstockinger
Copy link
Author

@taylorotwell I'll have a look, but probably won't get around to it until Friday. Hope that's all right?

@christophstockinger
Copy link
Author

Okay I found some time this evening and create a port for the vue-starter-kit laravel/vue-starter-kit#52

@tnylea tnylea added the Additional Discussion When a PR needs a little additional discussion before merging label Feb 28, 2025
@tnylea
Copy link
Contributor

tnylea commented Feb 28, 2025

Thanks @christophstockinger 👏

Will be discussing this with Taylor tomorrow and following up on this. Appreciate the PR.

@tnylea
Copy link
Contributor

tnylea commented Mar 3, 2025

@christophstockinger, would you mind resolving the current conflicts and then we can work on getting this PR merged in the next few days.

Really appreciate it 👍

@tnylea tnylea added Awaiting User Response Waiting for developers response and removed Additional Discussion When a PR needs a little additional discussion before merging labels Mar 3, 2025
@christophstockinger
Copy link
Author

@christophstockinger, would you mind resolving the current conflicts and then we can work on getting this PR merged in the next few days.

Really appreciate it 👍

@tnylea All conflicts are resolved


- name: Build Assets
run: npm run build
- name: Create SQLite Database

Choose a reason for hiding this comment

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

Is this still needed? By default phpunit uses sqlite in memory https://github.com/laravel/react-starter-kit/blob/main/phpunit.xml#L26

Copy link
Author

Choose a reason for hiding this comment

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

Hi @devinnived , thanks for your comment.
I didn't make any changes in this regard, I just adjusted the order here as this is necessary for a successful pipeline.

Unfortunately, I am not an expert in PHPUnit and cannot assess your change request. If you are of the opinion that it can be done differently, then I would suggest that you contribute this yourself via a PR.

Choose a reason for hiding this comment

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

I understand. I checked the workflow files and do not see the Create SQLite Database section

Copy link
Author

Choose a reason for hiding this comment

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

#52

@tnylea
Copy link
Contributor

tnylea commented Mar 7, 2025

Hey @christophstockinger,

I just responded in the Vue PR here (laravel/vue-starter-kit#52) with our thoughts on this. If you could do the same to this PR and add the tsc to the CI workflow that would be great.

Really appreciate it.

Copy link
Contributor

@tnylea tnylea left a comment

Choose a reason for hiding this comment

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

We won't want to add the tsc to the build step, but this would be great to add to the Github Workflow. Let me know if you can make those updates and we can get this merged in.

Appreciate it!

@christophstockinger
Copy link
Author

We won't want to add the tsc to the build step, but this would be great to add to the Github Workflow. Let me know if you can make those updates and we can get this merged in.

Appreciate it!

Unfortunately, I won't get to it for a few days. Hope that's all right?

I also have a few other thoughts on this topic, but I need to sort them out more precisely and will come back to them in the update.

@christophstockinger
Copy link
Author

@tnylea PR is updated.

I have introduced a new command type:check, which compiles and checks the typescript using tsc.

A few more thoughts: Currently, however, this only does something in the internal Github action to keep the starter kit type-safe. However, I would still question whether there is any benefit in using the starter kit if other pipelines (e.g. Envoyer or Laravel Cloud) are used and not Github Action. You may need to find a way to point out how this is used.

One of your arguments was also that people who are not familiar with Typescript may have problems with it. I agree with this and would therefore ask whether it wouldn't make more sense to provide a starter kit for Typescript or Javascript? If necessary, you can also include this in the laravel new command and then install the correct repo accordingly. This is how many Javascript frameworks such as next.js do it, for example.

@tnylea
Copy link
Contributor

tnylea commented Mar 19, 2025

Thanks @christophstockinger

I totally hear what you are saying. The hard part about offering a JS and TS version is the maintenance. In addition to the authentication options, we would have three different versions to maintain for each React/Vue. Then, we have two versions of the Livewire. So, in total, we would have eight versions to maintain. Perhaps we can find a better way to streamline this in the future, but for now, we're going to go with adding the typescript checker only in the CI.

Thanks for the latest update. Really appreciate the contribution 👍

@tnylea tnylea added Approved Approved for Merge and removed Awaiting User Response Waiting for developers response labels Mar 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Approved for Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants