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

Introduce a 'tailwindcss:upgrade' task #466

Merged
merged 3 commits into from
Jan 23, 2025
Merged

Conversation

flavorjones
Copy link
Member

  • rename install/tailwindcss.rb to install/install_tailwindcss.rb
  • new script install/upgrade_tailwindcss.rb which:
    • removes the "inter-font" CSS tag from the application layout
    • runs "npx @tailwindcss/upgrade@next --force" if npx is available
  • new integration test for upgrade

Child of #462

@flavorjones flavorjones force-pushed the flavorjones-v4-upgrade branch 3 times, most recently from 9e3edc7 to 65ad81c Compare January 23, 2025 18:39
@EricGusmao
Copy link
Contributor

As I mentioned in #450 , the update tool assumes postcss.config.js is at the current directory where it is executed.

And I noticed this line wasn't changed

postcss_path = rails_root.join("config/postcss.config.js")

Maybe we should change the update task to also move the postcss config file to the root path

change the previous line to

postcss_path = rails_root.join("postcss.config.js") 

And update the documentation related to postcss to reflect accordingly.

What do you think about this approach? Are there any potential drawbacks or edge cases I might have missed?

- rename install/tailwindcss.rb to install/install_tailwindcss.rb
- new script install/upgrade_tailwindcss.rb which:
  - removes the "inter-font" CSS tag from the application layout
  - comments out references to 'defaultTheme' in tailwind.config.js
  - runs "npx @tailwindcss/upgrade@next" if npx is available
- new integration test
@flavorjones flavorjones force-pushed the flavorjones-v4-upgrade branch from 65ad81c to 25010f3 Compare January 23, 2025 18:52
@flavorjones
Copy link
Member Author

@EricGusmao good catch. I'll add that to this PR.

This is where the tailwind upgrade tool expects it to be, because
that's the convention, apparently.
@flavorjones
Copy link
Member Author

@EricGusmao added a commit to handle the postcss file.

@EricGusmao
Copy link
Contributor

@flavorjones There’s one aspect that still concerns me a bit. If the user isn’t using PostCSS, some migrations (e.g., updating class names in the view files) will fail.

Additionally, in setups without JavaScript tooling, the update process may fail to fully migrate tailwind.config.js because the tool assumes that the imported packages (e.g., tailwind plugins) are installed via a package manager, allowing them to be called.

Do you think it’s worth addressing these scenarios?

@flavorjones
Copy link
Member Author

@EricGusmao If I knew how to address those scenarios, I might be willing to try. But I think there is a limit to how much we can reasonably automate, and the users will have to do some manual work.

In other words, I think this is "close enough to ship", and we can continue to make the upgrade task better if we figure out ways to address edge cases like the ones you're mentioning.

Is that a reasonable response?

@flavorjones flavorjones merged commit d86e74b into main Jan 23, 2025
17 checks passed
@EricGusmao
Copy link
Contributor

@flavorjones Yes, I completely agree.

@flavorjones flavorjones deleted the flavorjones-v4-upgrade branch January 23, 2025 19:29
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.

2 participants