-
Notifications
You must be signed in to change notification settings - Fork 443
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
Document a propshaft approach to JS/CSS #2160
base: main
Are you sure you want to change the base?
Conversation
Resolves ViewComponent#1064 Given Rails 8 has been released without transpilation or bundled JavaScript by default, this documentation update demonstrates how to use Stimulus and CSS in a ViewComponent without webpacker. A more terse demonstration would be possible without the view component arguments and/or less complicated Stimulus behaviour however that might be less instructive. One of the trickier aspects of binding Stimulus controllers is often determining the correct data-controller key to use, especially when namespaces and/or multi-word component names are involved which this example makes explicit. Approach 2 demonstrates the Stimulus behaviour but not the css, that would likely involve dartsass-rails
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋🏻 thanks for taking this on! As we don't use this approach at GitHub, might you be able to find someone you think would be a good reviewer of this change?
@@ -10,6 +10,10 @@ nav_order: 5 | |||
|
|||
## main | |||
|
|||
* Add documentation for JavaScript and CSS without webpacker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Add documentation for JavaScript and CSS without webpacker | |
* Add documentation for JavaScript and CSS without webpacker. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion Joel. It looks like Github uses a Catalyst approach instead of Stimulus. I have to admit, I wasn't aware of Catalyst at all and it's really interesting.
It helps explain the shadow root web component approach described in Encapsulating assets. Separately, that section appears to be unchanged since the original gem documentation was released. When I came across it, my immediate tendency was to overlook it (and the Webpacker section) given the Rails buzz around import maps / Stimulus / eliminating node atm. Perhaps there's an opportunity to mention Catalyst or contextualise Web Components a bit more in that section to help justify it's approach?
I believe it was @reeganviljoen who suggested documentation for this would be useful. I was blocked for awhile trying to figure this out in lieu of anointed documentation. Now that Rails defaults have been updated to remove Webpacker and include stimulus-rails, I suspect I'm not the only one per the activity in Issue #1064.
ViewComponent's Governance documentation suggests there are several folks with commit access to this gem. Does GitHub have a process for allocating reviewers to pull requests? I'm not very familiar with the project prior to this so wouldn't know who to approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkotchoff sorry I have been very bust lately, I will see if I can get some more 👀 on this
|
||
```ruby | ||
# config/application.rb | ||
config.assets.paths << "app/components" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one small issue I has with this config in the past is it digests everything in this path even your erb files, one way to solve this is to get all the files in components and remove any mot asset files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be interested in a better approach. I'm not sure how to avoid this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we do something like this?
config.assets.paths += Dir.glob("app/components/**/*.js")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to avoid autoloading all assets in that app/components directory but when I have tried various approaches like the one you have suggested, I've been unable to make that work.
What problems did you run into?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config.assets.paths
for the purpose of loading javascript with importmap-rails expects this to contain an array of directories, not files.
Theoretically, you could probably create a directory structure for your view components like:
app/components
├── ...
├── examples
| ├── hello_world_component
| | ├── hello_world_component_javascript
| | | ├── hello_world_component_controller.js
| | ├── hello_world_component.css
| | └── hello_world_component.html.erb
| └── hello_world_component.rb
├── ...
and then do something like:
config.assets.paths += Dir["app/components/**/"].select { |d| d.include?('javascript') }
but that doesn't seem particularly nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @camertron, I note the thumbs up here but I'm not sure how to interpret that in order to update the pull request. Are you suggesting we look to try and demonstrate a more complicated folder structure or are you suggesting you're now comfortable with the perhaps unavoidable usage of:
config.assets.paths << "app/components"
@joelhawksley this config checks out with one small issue, otherwise I think its 100%, and thanks @jkotchoff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this 😄
pin "@hotwired/stimulus-loading", to: "stimulus-loading.js" | ||
``` | ||
|
||
### Approach 1 - Default _app/components_ ViewComponent directory using named Stimulus controllers, no autoloading |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really like to only have one section. Since app/components is the conventional place to put components, let's keep this section and remove the other one. It looks like propshaft now supports putting stimulus controllers in app/components anyway, so I don't know if there's much use in documenting another approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also be super keen on showing a single approach. However, importing each stimulus controller with a named key:
import HelloWorldComponentController from "examples/hello_world_component/hello_world_component_controller"
application.register("examples--hello-world-component", HelloWorldComponentController)
is quite painful compared to autoloading them with:
`pin_all_from "app/frontend/components", under: "controllers", to: "components"`
and as far as I'm aware, the issue described here hasn't been resolved in order to allow autoloading of stimulus controllers without the 'frontend' style subdirectory. I just tried all these steps again on a new Rails 8.0.1 project but the problem described in that issue remains.
Seems like there's some patching to be done in a library somewhere. For the time being, I thought it would be useful to show both approaches and then remove one of them when this is fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm ok, any idea why rails/importmap-rails#192 didn't fix the problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case, it sounds like this needs to be fixed upstream for the ideal experience. My concern is that the two approaches are confusing (not your fault) - we should just pick one and recommend it without expecting devs to make a choice. Such is the Rails Way™.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why that didn't fix the problem. I've tried various approaches, maybe I'm missing something but it seems everyone is feeling the same pain per the issue this is addressing.
I'm not sure which approach would be best to demonstrate at the moment, they're both suboptimal. I see a lot of examples of folks using the frontend
subdirectory workaround however I really don't like that. I've gone with Approach 1 for now for my own project however I really don't like that either and would prefer to be able to use Approach 2 without the subdirectory 😂.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @camertron, I note the thumbs up however I'm not sure how to interpret that. Are you suggesting you're happy to stick with both examples for the time being until the issue is resolved or are you suggesting you would prefer just one approach to be documented and if so, which one?
|
||
```ruby | ||
# config/application.rb | ||
config.assets.paths << "app/components" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we do something like this?
config.assets.paths += Dir.glob("app/components/**/*.js")
@camertron, regarding #2160 (comment), I'd love to avoid autoloading all assets in that |
Resolves #1064
Given Rails 8 has been released without transpilation or bundled JavaScript by default, this documentation update demonstrates how to use Stimulus and CSS in a ViewComponent without Webpacker.
A more terse demonstration would be possible without the view component arguments and/or less complicated Stimulus behaviour however that might be less instructive.
One of the trickier aspects of binding Stimulus controllers is often determining the correct data-controller key to use, especially when namespaces and/or multi-word component names are involved which this example makes explicit.
Approach 1 outputs a stylesheet link tag into HTML potentially several times if a ViewComponent using this technique is re-used in a page several times. Aside from that affecting the aesthetic of the rendered source, it seems like a reasonable trade-off to ensure the ViewComponent is completely self-sufficient inclusive of styling in lieu of the
Encapsulating assets
approach. A sidecar loaded .css (or .scss) file seems a like a logical place to look for styling.Approach 2 demonstrates the Stimulus behaviour but not the css, that would likely involve dartsass-rails.