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

Adds support for propshaft (#2) #114

Merged
merged 2 commits into from
May 10, 2024
Merged

Adds support for propshaft (#2) #114

merged 2 commits into from
May 10, 2024

Conversation

jvillarejo
Copy link
Contributor

I am migrating my webapp to use propshaft and dartsass and removing sprockets as the asset pipeline.

This PR introduce a new AssetPropshaftProvider to resolve the assets instead of the AssetPipelineProvider.

Researching propshaft more I ended replicating a similar idea implemented in the Propshaft::Server class.

We need to provide roadie with the compiled stylesheet, so it can:

  • Reference the correct digested assets when using url(...) on the stylesheet.
  • Compile the whole stylesheet if sass is being used.

Now the integration_spec works without requiring the assets to be compiled.
It serves the compiled assets when running in development environment (which the dummy apps run)

I also added expectations for the body ``background-color: green` inlining.

It fixes #112

@PikachuEXE
Copy link
Collaborator

@jvillarejo
Copy link
Contributor Author

There are some files that appear on the linter when running bundle exec standardrb that I didn't change
This ones:

  lib/roadie/rails/mailer.rb:11:9: Style/ArgumentsForwarding: Use anonymous block arguments forwarding (`&`).
  lib/roadie/rails/mailer.rb:13:31: Style/ArgumentsForwarding: Use anonymous block arguments forwarding (`&`).
  lib/roadie/rails/options.rb:109:23: Performance/StringIdentifierArgument: Use `:"#{option}="` instead of `"#{option}="`.

¿Should I fix them too on this PR ?

@PikachuEXE
Copy link
Collaborator

Yes please

@jvillarejo
Copy link
Contributor Author

There is a problem here. standardrb mark as linter errors this ones:

lib/roadie/rails/mailer.rb:11:9: Style/ArgumentsForwarding: Use anonymous block arguments forwarding (`&`).
lib/roadie/rails/mailer.rb:13:31: Style/ArgumentsForwarding: Use anonymous block arguments forwarding (`&`).
lib/roadie/rails/options.rb:109:23: Performance/StringIdentifierArgument: Use `:"#{option}="` instead of `"#{option}="`.

Fixing the one in lib/roadie/rails/options.rb shouldn't present any problem.

But the anonymous block arguments forwarding is a feature added in ruby 3.1
If I change that, the library won't be compatible with previous ruby versions.

I don't think that's part of the PR. but I don't know why the linter is now failing.

What should we do?
I can ignore the errors with a comment.

@PikachuEXE
Copy link
Collaborator

Coz the lint job is run on ruby 3.2
Please ignore with config file
https://github.com/standardrb/standard#ignoring-specific-rules-in-files-and-globs
Better have one place to track all ignore rules (will be removed when support for earlier ruby versions dropped)

@jvillarejo
Copy link
Contributor Author

Done @PikachuEXE

I added .standard.yml file to ignore those errors: b6d8de1

- 'lib/roadie/rails/mailer.rb':
- Style/ArgumentsForwarding
- 'lib/roadie/rails/options.rb':
- Performance/StringIdentifierArgument
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing new line at the end again :P
I will add https://editorconfig.org and set insert_final_newline
You should setup your editor(s)/IDE(s) to follow https://editorconfig.org config if present (though I have never seen any project allowing missing new line
I will simply merge and fix it later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops sorry!

@PikachuEXE PikachuEXE merged commit 610fe6e into Mange:master May 10, 2024
6 checks passed
@PikachuEXE
Copy link
Collaborator

CI failing now due to actual path shorten
I have no idea if it's the default config changed or the setup in this project's testing causing it...
https://github.com/Mange/roadie-rails/actions/runs/9326040234

@jvillarejo
Copy link
Contributor Author

I will check it out.

@PikachuEXE
Copy link
Collaborator

PikachuEXE commented Jul 1, 2024

@jvillarejo Any progress?

Edit 1: rails/propshaft#173 ?

@jvillarejo
Copy link
Contributor Author

Sorry @PikachuEXE I been having some rough weeks. I had this on my TODO list.
I will try to do fix it this week.
Thanks for the link !

@jvillarejo
Copy link
Contributor Author

jvillarejo commented Jul 9, 2024

Hey @PikachuEXE I made the PR that fixes the tests: #115

Again, sorry for the delay. I had some very busy weeks.

@PikachuEXE
Copy link
Collaborator

It's simply test case no urgency at all! :)

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.

Has anyone tested Roadie with propshaft?
2 participants