Type checking in the build process

[christophstockinger]

ℹ️ Port of the react-starter-kit laravel/react-starter-kit#37

---

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!

[tnylea]

What would be the difference running npm run build && vite build --ssr vs vite build && vite build --ssr

LMK. Thanks.

[christophstockinger]

If you execute npm run build, npm run prebuild is automatically executed before the build.

[tnylea]

Aha, perfect. Thanks for letting me know.

[tnylea]

Hey @christophstockinger.

Thanks for the PR. So, if we add this as an NPM build step, why aren't you adding this to the CI workflow? Seems like if we are going to add it in, we might as well add it to the workflow file, right?

[christophstockinger]

Hey @christophstockinger.

Thanks for the PR. So, if we add this as an NPM build step, why aren't you adding this to the CI workflow? Seems like if we are going to add it in, we might as well add it to the workflow file, right?

More information you find here: https://docs.npmjs.com/cli/v8/using-npm/scripts

[JoeyMckenzie]

Is there any particular reason these declarations were included instead of the usual vite-env.d.ts like the original Breeze templates had? Something along the lines of:

/// <reference types="vite/client" />

interface ImportMetaEnv {
    readonly VITE_APP_NAME: string;
    [key: string]: string | boolean | undefined;
}

interface ImportMeta {
    readonly env: ImportMetaEnv;
    readonly glob: <T>(pattern: string) => Record<string, () => Promise<T>>;
}

within a resources/js/types/vite-env.d.ts file. I think this approach completely overrides anything declared in ImportMetaEnv and ImportMeta if it exists in vite/client, rather than merging it (like the above). I know this PR is just moving this bit to a separate file, so it might be a better question for @tnylea.

[christophstockinger]

@JoeyMckenzie I have just tried it out. You are right, the type declaration for vite/client is not needed. I have just adjusted the PR accordingly and removed it.

[tnylea]

Thanks, Christoph 👏

Will be doing some discussion with Taylor on this tomorrow, and we'll follow up. Appreciate it!

[christophstockinger]

@tnylea All conflicts are also resolved here.

[jrd-lewis]

What does the extra && quote do for that v-if where it's already checking if there is a quote?

[tnylea]

Not sure, @christophstockinger, do you happen to know why there is this extra && quote here. LMK

[tnylea]

Hey @christophstockinger,

Really appreciate the PR. So, after discussing this a bit more with the team, we decided that running tsc as part of the pre-build step may not be the best approach since it can sometimes get in the way, especially for those who aren’t as familiar with TypeScript.

It would be more beneficial to incorporate this check into CI so that any TypeScript issues are flagged during the PR process rather than blocking local builds.

Would you be willing to adjust this PR to move the TSC check to CI instead?

Thanks so much 😊

[tnylea]

Per notes in the previous comment, we don't want vue-tsc or the tsc command to be required when running the build command.

This would be great if added to the Github Workflow though, if you could make those changes and fix any file conflicts that would be great.

Thanks!

[sfatimah]

Hello sir,
I dont know if this is related to the PR but i got 1 problem highlighted in the tsconfig.json. Can help?

Cannot find type definition file for 'vue/tsx'.
The file is in the program because:
Entry point of type library 'vue/tsx' specified in compilerOptions

[christophstockinger]

@tnylea PR is updated.

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

The fix from #74 caused several problems in ssr.js. As far as I could understand it is a javascript hack, which is why I left a comment in the file why I deactivated eslint line by line and also use ts-ignore.

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.

[christophstockinger]

Hello sir, I dont know if this is related to the PR but i got 1 problem highlighted in the tsconfig.json. Can help?

Cannot find type definition file for 'vue/tsx'. The file is in the program because: Entry point of type library 'vue/tsx' specified in compilerOptions

Hi @sfatimah ,

I have to be honest, I'm not so deep into the Vue ecosystem anymore that I'm not so deeply convinced of the framework itself. But as far as I know, vue/tsx is part of vue and it contains the type definitions. Therefore you can simply add it to tsconfig.json and Typescript will look for the corresponding file itself.

[tnylea]

Thanks, @christophstockinger, for updating this to only run in the CI. Would you mind resolving the conflict with the ziggy.d.ts file, and then we can get this merged in?

Really appreciate the contribution.