Introduce Stimulus to replace jquery-rjs#9756
Conversation
| const taskResult = await API.wait_for_task(data.task_id) | ||
| console.log('reload: after wait_for_task: ', data) |
There was a problem hiding this comment.
@asirvadAbrahamVarghese I am stuck right here for some reason, and I really don't understand it. When this part of the code executes, it calls into API.wait_for_task. API.wait_for_task then blows up with
Error in reloadable#reload: TypeError: miqDeferred is not a function
wait_for_task api.js:66
reload reloadable_controller.js:33
...
What I don't understand is window.miqDeferred is a function if I call if from the Browser console, but inside of API.wait_for_task, it doesn't exist and I just can't understand why. It's some sort of JavaScript scoping thing I think? It feels like API.wait_for_task should be breaking everywhere, but it doesn't, so I must be missing something. Or perhaps, API.wait_for_task can only be called from within react or angular code...and if so, why?
There was a problem hiding this comment.
BTW, how I'm testing this is go to Overview -> Reports -> Reports -> Add a new report, then pick any Base, and add any one column, then go to Preview -> Generate Report Preview. The generate button is what I'm changing to be Stimulus based.
There was a problem hiding this comment.
In the application.js, you've added import "../controllers". Here, what happens is - ES6 "imports" are hoisted and execute before CommonJS "require()" statements. When api.js was imported by Stimulus controllers, the line const { miqDeferred } = window; (line 3 in api.js) captured undefined because miq_application.js (which defines window.miqDeferred ) hadn't run yet.
In api.js, try removing
const { miqDeferred } = window;
and replace const deferred = miqDeferred(); with const deferred = window.miqDeferred();
This accesses window.miqDeferred at call time (when the user clicks the button) rather than at module load time, ensuring it's always defined.
There was a problem hiding this comment.
Just curious, wouldn't calling miqDeferred directly work without explicitly referencing it from window in local scope? I mean, we don’t really need to reference window here at all. I believe JS should naturally resolve this from the global scope when there’s no local reference.
The call happens at runtime here (when API.wait_for_task() is actually called), and by then miq_application.js should already have run during app load, So miqDeferred should already be defined on the window object which should be then accessible globally.
There was a problem hiding this comment.
@elsamaryv That worked. Thanks! I'm past that problem and on to the next one but at least I'm not stuck.
There was a problem hiding this comment.
@asirvadAbrahamVarghese I can try that - I thought I had tried that combination before, but maybe I didn't.
| if (taskResult.task_results && this.hasContentTarget) { | ||
| // If task_results contains HTML, replace the content | ||
| if (typeof taskResult.task_results === 'string') { | ||
| this.contentTarget.innerHTML = taskResult.task_results |
There was a problem hiding this comment.
Nice, this looks great! wondering if we can also render React components directly from this level
There was a problem hiding this comment.
If the server is returning HTML that includes react components, it should work, unless there's some bootstrapping code that needs to run, but it should be possible to add in that bootstrapping code into the stimulus controllers.
| @@ -1,4 +1,4 @@ | |||
| #form_preview | |||
| #form_preview{"data-controller" => "reloadable", "data-reloadable-url-value" => url_for(:action => "show_preview", :id => "#{@edit[:rpt_id] || 'new'}"), "data-reloadable-target" => "content"} | |||
There was a problem hiding this comment.
How does Stimulus look up the data-reloadable-target attribute in the DOM?
There was a problem hiding this comment.
Oh, is it data-{registered-controller-name}-target?
There was a problem hiding this comment.
I think so, here reloadable is the controller
There was a problem hiding this comment.
Yes that's exactly it. https://stimulus.hotwired.dev/reference/targets
b81c0eb to
0238f70
Compare
| } | ||
|
|
||
| // poll for the task status | ||
| await API.wait_for_task(task_id) |
There was a problem hiding this comment.
This is different from how the old way worked, and I'm wondering if we should switch to the old way. The old way doesn't use the API. Instead it hits the wait_for_task endpoint and uses the /report/wait_for_task endpoint. I think this might be better in the case someone doesn't have access to the /api/tasks endpoint, they would have access to this /report/wait_for_task endpoint. I gotta play around with access. I might have to completely rework wait_for_task though, since it's a weird polling mechanism that relies on the client-side rjs to come back in.
There was a problem hiding this comment.
@Fryguy I feel like for this pr we should try to keep it the same as the old way. This would help avoid any possible issues, including the one you mentioned above with access and permissions. Also, if we rework wait_for_task would it still work for controllers that aren't using stimulus yet? This would allow us to do the conversion to stimulus in multiple smaller prs rather than all at once in one big pr.
There was a problem hiding this comment.
Also, if we rework wait_for_task would it still work for controllers that aren't using stimulus yet? This would allow us to do the conversion to stimulus in multiple smaller prs rather than all at once in one big pr.
Yes, that is my plan - not sure if I just want a new entrypoint method, or a parameter, but, yes, the idea is to allow this to be completed in small PRs
There was a problem hiding this comment.
Ok I've updated this to now work the "old" way by hitting the wait_for_task endpoint defined on the server side.
|
I've moved this out of Draft status. I think there's still more to do here to clean it up, but want to discuss the approach and my concern in #9756 (comment). I think I want to change a couple more controllers as well to make sure this is robust enough. |
e27bb40 to
1f74436
Compare
| require 'sprockets/railtie' | ||
|
|
||
| require 'high_voltage' | ||
| require 'stimulus-rails' |
There was a problem hiding this comment.
@Fryguy do we need this at require time? Note, it looks pretty lightweight right now but if they add dependency requires, we might have to watch out for this eager require at load time for all rails processes
https://github.com/hotwired/stimulus-rails/blob/main/lib/stimulus-rails.rb#L4-L5
https://github.com/hotwired/stimulus-rails/blob/main/lib/stimulus/engine.rb
There was a problem hiding this comment.
Is a good question. I think it was required, but I can't remember why exactly. I can try out loading it on demand, but maybe separate PR?
There was a problem hiding this comment.
sure, I vaguely recall trying to avoid eager requiring the cypress on rails related gems and trying to avoid that for all rails processes that have cypress-on-rails...
I found a way do it in the engine here so it was done early enough in the rails boot sequence but again, not at require time:
1191a31
|
|
||
| // Configure Stimulus development experience | ||
| application.debug = false | ||
| application.debug = (process.env.NODE_ENV === 'development') |
There was a problem hiding this comment.
note, if this changes the UI experience with stimulus, we might need to either make cypress use non-development or hook this so cypress mode disables debug
There was a problem hiding this comment.
It's only debug logging in the console (logging of stimulus events and changes)
jrafanie
left a comment
There was a problem hiding this comment.
LGTM, I think I follow some of what's happening here. I think it's a good first step. @Fryguy do you have a list of pages we can do as good next steps? Once we do some easier pages and have some experience, we might be able to tackle some of the harder ones.
|
Some lint errors in the JS files but I can fix those in a follow-up commit/pr if needed. Will merge once we decide if we want to fix those in this pr or follow up PR. |
|
I'll fix them |
| @@ -1,3 +1,6 @@ | |||
| // Stimulus | |||
| import '../controllers'; | |||
There was a problem hiding this comment.
Will this functionally changes things now that it's on the top? I had it on the bottom on purpose because it depended on other things that were required.
There was a problem hiding this comment.
Seems fine to me
|
Checked commits Fryguy/manageiq-ui-classic@71d3bc6~...17b73c4 with ruby 3.3.10, rubocop 1.86.0, haml-lint 0.72.0, and yamllint 1.37.1 app/views/report/_form_preview.html.haml
|
|
@asirvadAbrahamVarghese Can you review and merge? Gilbert added a commit here otherwise he'd merge. |
asirvadAbrahamVarghese
left a comment
There was a problem hiding this comment.
Looks great, thanks
This PR introduces Stimulus as a temporary replacement for jquery-rjs, and also changes a few places to demonstrate how we can use it.
I've verified this works when the task is not yet finished by killing my simulate_queue_worker for a bit...it continues to poll until the simulator is reenabled and the work is actually completed.
Part of #6521