Skip to content

Conversation

andresg4
Copy link
Contributor

Board:


Description:

The sass-rails gem is deprecated, in this PR we use cssbundling-rails and configure dartsass with it


Notes:


Tasks:

  • Add each element in this format

Risk:


Preview:

@andresg4 andresg4 requested review from a team, JulianPasquale, PerezIgnacio and santib August 14, 2024 19:46
Copy link
Member

@santib santib left a comment

Choose a reason for hiding this comment

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

Niceeeee

@santib santib requested a review from a team August 14, 2024 19:50
gem 'sass-rails', '~> 6.0.0'
gem 'sendgrid', '~> 1.2.4'
gem 'sprockets', '~> 4.2.1'
gem 'sprockets-rails', '~> 3.5', '>= 3.5.2'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this changing from sprockets to sprokets-rails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we keep the sprockets gem and remove the sass-rails, we get this error 'require': cannot load such file -- sprockets/railtie. Replacing it with the sprockets-rails one solves it. I'm not really sure why this happens

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to understand why and what is the impact of this change. I assume we're good, but let's validate it doesn't have any negative impact

Copy link
Member

Choose a reason for hiding this comment

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

just in case, sprockets-rails was already being installed as a dependency on sass-rails. https://github.com/rootstrap/rails_api_base/pull/807/files#diff-89cade48462044ee1b672dc5f4c3ec250fbd29effcd8932096a23c1283c6731fL571-L573

Basically our previous setup wasn't so good, that's the whole issue here. sprockets-rails is required for our app that we are using the traditional asset pipeline.

"scripts": {
"build": "node esbuild.config.mjs"
"build": "node esbuild.config.mjs",
"build:css": "sass ./app/assets/stylesheets:./app/assets/builds --no-source-map --load-path=node_modules"
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the build script run this as well? Maybe we can call this prebuild so it runs automatically?

Copy link
Member

Choose a reason for hiding this comment

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

I think this was added by the cssbundling gem. I mean, the default behavior.

Copy link
Contributor

@JulianPasquale JulianPasquale Aug 15, 2024

Choose a reason for hiding this comment

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

Yeah, I'm just saying that would be nice to have something like

"scripts": {
  "build": "yarn build:js && yarn build:css",
  "build:js": "node esbuild.config.mjs",
  "build:css": "sass ./app/assets/stylesheets:./app/assets/builds --no-source-map --load-path=node_modules"
}

That way build runs both process. Or another option would be to rename the build:css script to prebuild so it's always executed when we call build. (I like the first option because it's more flexible tbh).

Anyway, if you want to keep it this way, we might need to add the build:css step to our Dockerfile.dev

Copy link
Contributor

@PerezIgnacio PerezIgnacio left a comment

Choose a reason for hiding this comment

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

Looks good! Do we need any updates on fullstack script? (which installs tailwind and replaces build:css)

@andresg4 andresg4 force-pushed the remove-sass-rails-gem branch from 6a6dd4a to 25a3a1d Compare August 15, 2024 14:33
@andresg4 andresg4 merged commit fca1ff3 into main Aug 16, 2024
@andresg4 andresg4 deleted the remove-sass-rails-gem branch August 16, 2024 17:26
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.

6 participants