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

Skip CSS when creating APIs. #50907

Merged
merged 1 commit into from Jan 29, 2024
Merged

Conversation

ruyrocha
Copy link
Contributor

Motivation / Background

This Pull Request has been created because Tailwind CSS was included when generating APIs.

Detail

This Pull Request changes the behavior of skipping CSS for APIs.

Additional information

It addresses part of #50900.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the railties label Jan 28, 2024
Copy link
Member

@skipkayhil skipkayhil left a comment

Choose a reason for hiding this comment

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

My first instinct is that we shouldn't allow conflicting options to be specified, so I'm not sure if this should be accepted. However, in case someone else sees it differently I left a few comments on the implementation

Edit: and once my suggestions are addressed, you should squash your commits into one

@@ -309,6 +309,7 @@ class AppGenerator < AppBase
api: [
:skip_asset_pipeline,
:skip_javascript,
:skip_css
Copy link
Member

Choose a reason for hiding this comment

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

There is currently no --skip-css option, so I don't think this is doing anything

Comment on lines 104 to 106
assert_no_file "app/views/layouts/applicaearion.html.erb" do |content|
assert_no_match(/tailwind/, content)
end
Copy link
Member

Choose a reason for hiding this comment

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

The filename here is misspelled. Additionally, since the expectation is that there is no file I don't think the block is ever run.

Update CHANGELOG.

Address requested changes.
@ruyrocha
Copy link
Contributor Author

Thank you so much, @skipkayhil! The changes have been addressed.

The edge case happens when css is already set on ~/.railsrc file, so it fails.

@rafaelfranca rafaelfranca merged commit 44e2bc2 into rails:main Jan 29, 2024
4 checks passed
@bradleypriest
Copy link
Contributor

@ruyrocha wouldn’t the —no-rc flag to rails new have been a better option, seems like that was the root cause of the issue.

@ruyrocha ruyrocha deleted the fix/skip-css-on-apis branch January 29, 2024 19:53
@ruyrocha
Copy link
Contributor Author

@ruyrocha wouldn’t the —no-rc flag to rails new have been a better option, seems like that was the root cause of the issue.

I'm afraid they're separate issues, @bradleypriest

@bradleypriest
Copy link
Contributor

I feel like if I wrote rails new example --css --api, I would be surprised that there was no CSS generated, I can't think of a use-case, but if you're explicitly passing those flags, it would be surprising. Technically that's what this has changed right?

Isn't the only reason this came up in reality because it also involves the rc file?

@ruyrocha
Copy link
Contributor Author

Got it.

It started with rc file issue, and

  • there is --no-rc to ignore it at all 🚀

  • it seems the comments are now ignored on rc file 🚀

  • and CSS will be skipped when generating APIs 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants