use actioncable to track indexing progress #3182
Conversation
dnoneill
force-pushed
the
ws-progress
branch
3 times, most recently
from
5f4c782
to
3858c27
Compare
| <%= t("#{translation_field}.error") %> | ||
| </p> | ||
|
|
||
| <div class="progress"> |
There was a problem hiding this comment.
Could use use the <progress> element? https://developer.mozilla.org/en-US/docs/Web/HTML/Element/progress
There was a problem hiding this comment.
No. This is a bootstrap class: https://getbootstrap.com/docs/4.0/components/progress/. Changing this to it breaks the CSS.
There was a problem hiding this comment.
Isn't the semantic element better for accessibility than a bootstrap component?
There was a problem hiding this comment.
Probably. I think if we are trying to solve the accessibility issue then we should open a new ticket. This is the progress bar that was used in app/views/spotlight/catalog/_reindex_progress_panel.html.erb and app/views/spotlight/bulk_updates/_progress_panel.html.erb
| File.open(csv_path) do |f| | ||
| progress&.total = f.each_line.count(&:present?) - 1 # ignore the header | ||
|
|
||
| ::CSV.table(f, header_converters: header_converter).each do |row| | ||
| process_row(exhibit, row) | ||
| progress&.increment | ||
| ActionCable.server.broadcast 'progress_channel', |
There was a problem hiding this comment.
Should this be a different channel from the one the indexing uses?
There was a problem hiding this comment.
They use the same JS file/functions. I would need to update a bunch of JS.
There was a problem hiding this comment.
To be clear, I'm not saying that it should be. I'm just asking whether you considered it and why you decided that one channel is the best approach.
There was a problem hiding this comment.
I considered it. Seemed like I would just be adding another listener for a different channel that would do the exact same thing.
| @@ -29,5 +30,7 @@ | |||
| "not IE 11" | |||
| ], | |||
| "dependencies": { | |||
| "@rails/actioncable": "^7.2.100", | |||
There was a problem hiding this comment.
Won't this depend on the rails version the host app is using?
There was a problem hiding this comment.
You are suggesting I update the version to be a range?
There was a problem hiding this comment.
We want to allow rails 7.1 and rails 8.0 (eventually), right? I think some kind of range would be preferable.
There was a problem hiding this comment.
Yeah I just wanted to make sure I understood the suggestion and it wasn't something else.
| @@ -24,7 +25,7 @@ const rollupConfig = { | |||
| name: ESM ? undefined : 'Spotlight' | |||
| }, | |||
| external, | |||
| plugins: [includePaths(includePathOptions)] | |||
| }; | |||
| plugins: [includePaths(includePathOptions), nodeResolve()] | |||
There was a problem hiding this comment.
Wouldn't work without it.
There was a problem hiding this comment.
What was the nature of the failure?
There was a problem hiding this comment.
(!) Unresolved dependencies
https://rollupjs.org/troubleshooting/#warning-treating-module-as-external-dependency
@rails/actioncable (imported by "app/javascript/spotlight/channels/consumer.js")
created app/assets/javascripts/spotlight/spotlight.esm.js in 184ms
| @@ -0,0 +1 @@ | |||
| // Import all the channels to be used by Action Cable | |||
There was a problem hiding this comment.
It is automatically created by ActionCable. I wasn't sure if it was something we would want to keep.
There was a problem hiding this comment.
Automatically created into an engine? How does that work?
There was a problem hiding this comment.
In a site. I created the site ran the auto creation task and moved everything.
There was a problem hiding this comment.
You don't want to move these files into the engine. They should be generated by the auto creation task in the host app.
|
Does this impose a new Redis dependency on everyone who wants to run Spotlight? |
|
@jcoyne Yeah but the way I understood it, it was already a dependency of spotlight? |
|
@dnoneill redis is not a dependency of spotlight as far as I know. |
| // Action Cable provides the framework to deal with WebSockets in Rails. | ||
| // You can generate new channels where WebSocket features live using the `bin/rails generate channel` command. | ||
|
|
||
| import { createConsumer } from "@rails/actioncable" |
There was a problem hiding this comment.
I don't think this file belongs in the engine. It should be in the host application. By doing that you can remove the @rollup/plugin-node-resolve
There was a problem hiding this comment.
You are suggesting this goes in the install generator?
There was a problem hiding this comment.
Isn't it already in the actioncabel generator?
There was a problem hiding this comment.
Yes. Okay so you are suggesting I put the
rails generate channel NameOfChannelinto the install generator. Remove app/channels/application_cable/channel.rb, app/channels/application_cable/connection.rb, and app/javascript/channels/consumer.js.- Either make the name of channel ProgressChanel and update the app/channels/progress_channel.rb to include the right stream in the generator or make it a random channel that doesn't get used
I am guessing for existing apps we are going to need to include instructions on how to install. I am not quite sure how this works with existing apps.
There was a problem hiding this comment.
Yes, that is the path I would use. Yes, we would need instructions for how to install this. It should include something like "Install and configure Redis and actioncable" (assuming we're not doing Solid + Rails 8).
|
@dnoneill could this please link to an issue that says what / why |
|
moving into draft. Got 90% of the way to moving all the actioncable stuff into the engine. Unfortunately we couldn't figure out how to import channels/consumer into sprockets without it erroring. It also checks for actioncable, if not falls back to previous behavior. |
closes #3049