Migrate Web Apps to RequireJS #34271
Conversation
There was a problem hiding this comment.
Got to 76b6fe8. Waiting for a response so I don't make stuff unnecessarily.
| 'backbone.marionette', | ||
| 'cloudcare/js/formplayer/app', | ||
| 'cloudcare/js/formplayer/utils/utils', | ||
| 'bootstrap-switch/dist/js/bootstrap-switch', // bootstrapSwitch |
There was a problem hiding this comment.
So in this case what brought the bootstrap-switch dependency in before?
There was a problem hiding this comment.
That one is a jquery plugin, so it adds an attribute to jquery rather than its own global, so ESLint doesn't notice it.
| @@ -109,7 +109,7 @@ <h3><%- displayText ? displayText : "" %></h3> | |||
| <div class="page-header menu-header"> | |||
| <h1 class="page-title"><%- title %></h1> | |||
| </div> | |||
| <div class="<% if (environment === hqImport("cloudcare/js/formplayer/constants").PREVIEW_APP_ENVIRONMENT) { %>container<% } %>"> | |||
| <div class="<% if (isAppPreview) { %>container<% } %>"> | |||
There was a problem hiding this comment.
Would you say in general that it is best practice to rely on variable instead of function calls/imports inside the templates?
There was a problem hiding this comment.
yes, definitely
| _, | ||
| Marionette, | ||
| moment, | ||
| DOMPurify, |
There was a problem hiding this comment.
DOMPurify does not seem to be used in this file.
There was a problem hiding this comment.
thanks, fixed in 3460c5b
| 'cloudcare/js/formplayer/users/controller', | ||
| 'cloudcare/js/formplayer/users/models', | ||
| 'marionette.approuter/lib/marionette.approuter.min', // for Marionette.AppRouter | ||
| 'cloudcare/js/formplayer/sessions/api', // for getSession |
There was a problem hiding this comment.
was resolving conflict on staging and noticed, is this needed? I think getSession is defined within router.js
There was a problem hiding this comment.
Ah, I always thought just importing where the channel is defined cloudcare/js/formplayer/app was enough. Thanks for explaining
orangejenny
requested review from
AddisonDunn,
stephherbers,
Robert-Costello and
nospame
as code owners
orangejenny
added
QA Passed
and removed
awaiting QA
Technical Summary
Does what the title says.
I'd recommend reviewing by commit. This is big, but most commits follow a similar pattern.
FAQ on RequireJS migration
Nope. Web apps, app manager, and reports still use script tags 🎶 like it's 1999 🎶
Yep. This is still a step forward, since it makes all of the js dependencies in web apps explicit, which any modern module approach will expect. It is also a valuable endeavor to move our js dependency infrastructure to use modern tooling, but I am not blocking this work on that.
Yep. This is the sad result of requirejs depending on a version of uglify that depends on an old version of esprima. See here. At least, I'm certain that's the problem with the spread syntax and suspect it's the same for optional chaining (EDIT: looks like it is). In third party libraries that are already minified, we can work around this by using empty: to skip optimization. For our own code, in theory we should be able to use babel-requirejs to work around it, but full disclosure I have not gotten that working and have been reduced to removing a small amount of that kind of code rather than blocking this migration on that problem.
Safety Assurance
Safety story
High risk. I'll coordinate with USH on testing and deploy timing.
Automated test coverage
Some. All web apps js tests are relevant here, but their coverage isn't that high.
QA Plan
Web apps UI regression: https://dimagi.atlassian.net/browse/QA-6193
Rollback instructions
Labels & Review