-
Notifications
You must be signed in to change notification settings - Fork 66
Adjust install generator for typescript setups #217
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: master
Are you sure you want to change the base?
Adjust install generator for typescript setups #217
Conversation
62fb7f4
to
b23e8aa
Compare
dfbbe06
to
9e226ae
Compare
@@ -125,6 +125,13 @@ def install_typescript | |||
return | |||
end | |||
|
|||
if File.exist?(application_js_path) && application_layout.read.include?("<%= vite_javascript_tag 'application' %>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can’t find the application_js_path
, we skip making any changes.
This likely means the user has customized their setup or didn’t follow the full generator path — so we avoid modifying anything to prevent interfering with their configuration.
e452da9
to
be6dee5
Compare
be6dee5
to
6463fba
Compare
hey @bknoles, in case this PR looks good to you, could you help merging this? I'm not so familiar with contributing to OSS so Im sorry for directly mentioning you, but Im not sure how to properly handle these cases when it comes down to opening a PR, assign or mention someone etc... |
@Andy9822 thanks for the PR! I wonder what happens if a user installs Inertia Rails into an existing app that already uses |
Yea, I'd say Since |
hmm, not sure Im following what issue you're pointing out here @skryukov 🤔 The code of this PR runs inside the ![]() I personally don’t think we need to take an "extreme" or so opinionated measure like deleting the application.js file if it’s generated by Inertia — as @bknoles suggested — since it’s actually a fine and useful part of the install command, which helps scaffold a basic structure for you. It’s similar to what you get when generating a new Rails app with its install command. |
I'm talking about a case when someone installs Inertia Rails on top of an existing application (not a |
aa6f9d8
to
4be55da
Compare
oooh ok, you mean when > Vite < had been installed outside of the Inertia install generator, sorry I finally got what you meant! |
83e0a3f
to
1299957
Compare
Users may install Inertia on projects that already have Vite installed. Now, the renaming only occurs when the Inertia generator is also installing Vite, ensuring compatibility with existing Vite setups and preventing unnecessary file renaming.
1299957
to
a197826
Compare
This PR updates the Inertia install generator to use
vite_typescript_tag "application"
instead ofvite_javascript_tag
on the application layout when the--typescript
flag is passed.Additionally, it renames the generated
application.js
file toapplication.ts
to align with the expected TypeScript setup.This ensures consistency with the rest of the setup—for example, the inertia entrypoint already uses
vite_typescript_tag
. It also prevents confusion or broken asset references if developers try to update the application layout based on the included comment suggesting the use ofvite_typescript_tag
:If they do switch to
vite_typescript_tag 'application'
, as per the generated comment above, the asset fetching will fail because we generated the file asapplication.js
even though it's a Typescript setupNew behavior
Closes #218