Update jQuery / Bootstrap, remove Fluid Infusion#514
Update jQuery / Bootstrap, remove Fluid Infusion#514Naenyn wants to merge 2 commits intouPortal-Project:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes client-side library usage by removing legacy, portlet-bundled jQuery assets (and related resource-server skin aggregation) and shifting JSPs to rely on portal-provided jQuery (up.jQuery) and WebJars-based assets.
Changes:
- Removed legacy Resource Server
skin.xmljQuery/jQueryUI aggregation descriptor. - Removed the vendored
jquery.form.jsplugin. - Updated multiple JSPs to stop conditionally loading old
/rs/jquery/1.11.0/...resources and instead use portal-providedup.jQuery(or other updated sourcing).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/webapp/skin.xml | Deleted legacy resource-server skin aggregation file for jQuery/jQueryUI. |
| src/main/webapp/js/jquery.form.js | Deleted vendored jQuery Form Plugin file. |
| src/main/webapp/WEB-INF/jsp/cms/configureContent.jsp | Removed skin.xml aggregated include and simplified jQuery selection to up.jQuery fallback. |
| src/main/webapp/WEB-INF/jsp/attachments/main.jsp | Removed conditional jQuery include; changed jQuery selection logic to use portal namespace variable. |
| src/main/webapp/WEB-INF/jsp/attachments/ckeditor-callback.jsp | Switched jQuery include to a hard-coded /resource-server/webjars/... path. |
| src/main/webapp/WEB-INF/jsp/attachments-manager/view.jsp | Removed conditional jQuery include and updated ready handler to use namespaced jQuery. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ${n}.jQuery = ${ ns }jQuery; | ||
| </c:otherwise> | ||
| </c:choose> | ||
| ${n}.jQuery = (typeof ${portalJsNamespace} !== 'undefined' && ${portalJsNamespace}.jQuery) ? ${portalJsNamespace}.jQuery : jQuery; |
| <%-- rs:resourceURL resolves to /ResourceServingWebapp by default; webjars live under /resource-server --%> | ||
| <script src="/resource-server/webjars/jquery/dist/jquery.min.js" type="text/javascript"></script> |
| <%-- jQuery is now provided by the portal as up.jQuery; skin.xml include removed --%> | ||
| <script src="<rs:resourceURL value='/rs/ckeditor/4.3.2/ckeditor.js'/>" type="text/javascript"></script> |
| <c:if test="${!usePortalJsLibs}"> | ||
| <script src="<rs:resourceURL value='/rs/jquery/1.11.0/jquery-1.11.0.min.js'/>" type="text/javascript"></script> | ||
| </c:if> | ||
| <%-- jQuery is now provided by the portal as up.jQuery --%> |
bjagg
left a comment
bjagg
left a comment
There was a problem hiding this comment.
This appears to be the same diff as the closed #513. The feedback from that review still applies — reposting the key items here.
Critical
🐛 Regression in attachments/main.jsp — ${portalJsNamespace} interpolation
${n}.jQuery = (typeof ${portalJsNamespace} !== 'undefined' && ${portalJsNamespace}.jQuery) ? ${portalJsNamespace}.jQuery : jQuery;${portalJsNamespace} is a JSP expression. If that preference is empty at render time, this becomes:
Pluto_x_.jQuery = (typeof !== 'undefined' && .jQuery) ? .jQuery : jQuery;That's a JavaScript syntax error that breaks the entire script block. The original code handled the empty case explicitly with <c:if test="${ not empty portalJsNamespace }">.
Suggested fix — use the same hardcoded up pattern that configureContent.jsp already uses in this PR:
${n}.jQuery = (typeof up !== 'undefined' && up.jQuery) ? up.jQuery : jQuery;⚠️ Inconsistent fallback patterns across the same PR
Three different patterns:
attachments-manager/view.jsp:(typeof up !== 'undefined' && up.jQuery) ? up.jQuery : jQueryattachments/main.jsp:(typeof ${portalJsNamespace} !== 'undefined' && ${portalJsNamespace}.jQuery) ? …(broken — see above)cms/configureContent.jsp:(typeof up !== 'undefined' && up.jQuery) ? up.jQuery : jQuery
Pick one. Hardcoded up matches convention used elsewhere and avoids the empty-variable regression.
⚠️ Bare jQuery fallback is dangerous
The : jQuery branch in all three patterns assumes jQuery is a defined global. If neither up.jQuery nor a bundled jQuery is loaded, jQuery is a ReferenceError (not undefined), so the ternary throws before the assignment. Either trust that up.jQuery is always present and drop the fallback, or guard with typeof jQuery !== 'undefined' ? jQuery : null.
Medium
Dead portlet preferences in portlet.xml
includeJQuery and Attachments.usePortalJsLibs are still defined in portlet.xml but are no longer read by any JSP after this PR. Either clean them up here or file a follow-up.
Hardcoded /resource-server/webjars/jquery/dist/jquery.min.js in ckeditor-callback.jsp
Brittle if resource-server is deployed at a different context path. Why not keep <rs:resourceURL>?
Verify jquery.form.js deletion
Worth a quick grep for ajaxForm, ajaxSubmit, or jQuery.form in the codebase before merging to confirm nothing still depends on it.
resource-server-utils still needed?
configureContent.jsp still uses <rs:compressJs> and rs:resourceURL for CKEditor — so resource-server-utils needs to stay in the POM. The PR doesn't touch pom.xml, which means that's implicitly the case. Worth confirming.
Minor
- Empty PR body — a short summary + test plan would help reviewers.
No description provided.