这是indexloc提供的服务，不要输入任何密码
Skip to content

Improve js and css options of app generator #47689

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 25, 2023
Merged

Improve js and css options of app generator #47689

merged 1 commit into from
May 25, 2023

Conversation

@javierav
Copy link
Contributor

@javierav javierav commented Mar 16, 2023 •
edited
Loading

Motivation / Background

This Pull Request has been created because the Rails app generator currently has two issues:

First issue

If I choose importmap as the javascript option and bootstrap, bulma or postcss as the css option, rails forces me to use esbuild as the javascript option, ignoring my choice of importmap.

There should be no problem using importmap as a javascript option and using bootstrap, bulma or postcss with Node.js

Second issue

If I choose importmap as the javascript option, the .node-version file is not created and the Dockerfile does not include the Node.js installation steps, even when Rails forces it to use esbuild (first issue) or if we are using a css option that requires Node.js

Detail

This Pull Request changes the requirements for the .node-version file to be created and the Dockerfile to include the Node.js installation.
It also changes the old behavior and if I choose importmap it uses it regardless of the css option selected.

Actual behavior

options[:javascript] options[:css] gems install commands require node install node status
importmap bootstrap, bulma, postcss jsbundling-rails
cssbundling-rails		 javascript:install:esbuild
css:install:*		 YES NO bug1
importmap tailwind importmap-rails
tailwindcss-rails		 importmap:install
tailwindcss:install		 NO NO ok
importmap sass importmap-rails
dartsass-rails		 importmap:install
dartsass:install		 NO NO ok
webpack, esbuild, rollup * jsbundling-rails
cssbundling-rails		 javascript:install:*
css:install:*		 YES YES ok

Proposed behavior

options[:javascript] options[:css] gems install commands install node
importmap bootstrap, bulma, postcss importmap-rails
cssbundling-rails		 importmap:install
css:install:*		 YES
importmap tailwind importmap-rails
tailwindcss-rails		 importmap:install
tailwindcss:install		 NO
importmap sass importmap-rails
dartsass-rails		 importmap:install
dartsass:install		 NO
webpack, esbuild, rollup * jsbundling-rails
cssbundling-rails		 javascript:install:*
css:install:*		 YES

Still, there is a combination that is not possible: if I want to use importmap and cssbundling-rails to compile the css using tailwind or sass instead of use tailwindcss-rails or dartsass-rails.

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.

Footnotes

  1. no install node (required by jsbundling-rails) and change importmap to esbuild ↩

@rails-bot rails-bot bot added the railties label Mar 16, 2023
@javierav javierav mentioned this pull request Apr 11, 2023
Merged
4 tasks
jonathanhefner
jonathanhefner reviewed May 23, 2023
View reviewed changes
Copy link
Member

@jonathanhefner jonathanhefner left a comment

Choose a reason for hiding this comment

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

This makes sense to me, but it would be good to add some tests.

I am pushing a commit to add test coverage and address my comments. Let me know what you think.

railties/lib/rails/generators/app_base.rb Outdated
Copy link
Member

@jonathanhefner jonathanhefner May 23, 2023

Choose a reason for hiding this comment

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

options[:javascript] does have a default value ("importmap"), but specifying --no-javascript would set it to nil, so let's keep the presence check.

Copy link
Contributor Author

@javierav javierav May 24, 2023

Choose a reason for hiding this comment

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

Good catch!

railties/lib/rails/generators/app_base.rb Outdated
Comment on lines 580 to 570
Copy link
Member

@jonathanhefner jonathanhefner May 23, 2023

Choose a reason for hiding this comment

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

I think using_node? is the correct condition here, and it makes the intent more clear. Is there a reason to not use it?

Copy link
Contributor Author

@javierav javierav May 24, 2023

Choose a reason for hiding this comment

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

Look good for me! I have used form options[:javascript] == "importmap" because it has been a literal translation of the PR table, but it seems to me a successful change.

@javierav @jonathanhefner
Improve js and css options of app generator
84458a8 
This allows the `importmap-rails` gem to be used when using the
`cssbundling-rails` gem.  It also ensures that Node-related files (e.g.
`.node-version`) are added when using the `cssbundling-rails` gem.

Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>
@jonathanhefner jonathanhefner merged commit f96fd4e into rails:main May 25, 2023
@jonathanhefner
Copy link
Member

jonathanhefner commented May 25, 2023

Thank you, @javierav! 📦

@javierav javierav deleted the feature/improve-css-javascript branch May 25, 2023 17:01
@skipkayhil skipkayhil mentioned this pull request Jan 9, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jonathanhefner jonathanhefner jonathanhefner left review comments

Assignees

No one assigned

Labels

railties

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@javierav @jonathanhefner