Refactor admin JavaScript build system and structure#2464
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughReworks build/lint config and admin bundling, updates admin style imports, applies widespread JavaScript formatting (many files), converts several IIFE wrappers to arrow IIFEs, and includes a small set of functional tweaks (doJsonPost fallback, class-counter guard fix, square frontend submit logic, applicationTemplates now returns void). Minor PHP reorder and new .browserslistrc added. Changes
Sequence Diagram(s)(No sequence diagram provided — changes are mostly formatting, packaging, and small isolated logic tweaks; no single new control-flow feature to visualize.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.eslintignore (1)
9-9: Use a glob for consistency and future-proofing.Other generated bundles here use a recursive glob. Recommend switching to a double-star pattern so path changes or subdirectory outputs won't require another edit.
- js/formidable_admin.js + **/formidable_admin.jswebpack.config.js (1)
36-37: Confirm admin bundle enqueue; optionally externalize WP packagesWe’ve verified that:
- The entry file
js/src/admin/admin.jsexists.- In
classes/controllers/FrmAppController.php:
wp_register_script( 'formidable_admin', ... '/js/formidable_admin.js', … );(line 710)wp_enqueue_script( 'formidable_admin' );(line 738)Optional: to avoid bundling WP packages imported by your admin code (e.g.
@wordpress/i18ninstyles.js), add them tosharedConfig.externals. For example:--- webpack.config.js @@ const sharedConfig = { - externals: { - jquery: 'jQuery', - }, + externals: { + jquery: 'jQuery', + '@wordpress/i18n': [ 'wp', 'i18n' ], + },This will let WordPress core provide the i18n module, reducing your bundle size.
js/src/admin/styles.js (2)
2-5: Good move to components/; consider centralizing exports to simplify imports.As the components list grows, a barrel file reduces import noise and improves discoverability.
Apply this diff to consolidate imports:
-import frmRadioStyleComponent from './components/radio-style-component.js'; -import frmSliderStyleComponent from './components/slider-style-component.js'; -import frmTabsStyleComponent from './components/tabs-style-component.js'; -import frmStyleDependentUpdaterComponent from './components/dependent-updater-component.js'; +import { + frmRadioStyleComponent, + frmSliderStyleComponent, + frmTabsStyleComponent, + frmStyleDependentUpdaterComponent +} from './components';Then add js/src/admin/components/index.js with:
export { default as frmRadioStyleComponent } from './radio-style-component.js'; export { default as frmSliderStyleComponent } from './slider-style-component.js'; export { default as frmTabsStyleComponent } from './tabs-style-component.js'; export { default as frmStyleDependentUpdaterComponent } from './dependent-updater-component.js';Also, because this file imports from @wordpress/i18n, ensure webpack treats it as an external (wp.i18n) to avoid bundling WP packages; see suggestion in webpack.config.js.
21-21: Nit: clarify the JSDoc sentence.Small readability tweak.
- * Init the dependent + * Initialize the dependent updater components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.eslintignore(1 hunks)js/src/admin/styles.js(2 hunks)webpack.config.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: Cypress
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Run PHP Syntax inspection (8.3)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
🔇 Additional comments (2)
webpack.config.js (1)
145-149: LGTM: dev-only source maps in CSS pipeline.The css-loader options are correct for disabling url() handling and limiting source maps to development. No changes requested.
js/src/admin/styles.js (1)
2-5: All component import paths verified—no missing files
The sanity check confirms that each of the imported files in js/src/admin/styles.js exists under js/src/admin/components:
- radio-style-component.js
- slider-style-component.js
- tabs-style-component.js
- dependent-updater-component.js
No build failures expected from missing or mis-cased filenames.
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
@shervElmi We need to make sure the new file is being linted as well. I don't want more files that aren't getting validated.
It looks like I can add ./node_modules/.bin/eslint ./js/src/admin/admin.js to our jscs.yml, but if I try to add another file, like ./node_modules/.bin/eslint ./js/src/styles.js, it seems to have issues. I think because of the import lines.
I see this error instead:
Cannot find module '.../wp-content/plugins/formidable/node_modules/@wordpress/i18n/webpack.config.js
Could you help with fixing that?
|
@Crabcyborg how about linting all JS files in the We can then exclude the auto-generated files by adding them to |
…checkboxes_when_limit_is_reached Auto-disable screen options checkboxes when limit is reached
|
@shervElmi That sounds good, but since several files weren't being linted, it may not be super easy to do it all at once. There is likely code throwing errors. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
classes/controllers/FrmEntriesController.php (1)
448-452: Tighten docblock types for better static analysisSpecify the expected
$attsshape, note pass-by-reference for$result, and declare the global used. This helps IDEs and tooling.- * - * @param array $atts - * @param array $result - * @return void + * @param array{ i:int, max_columns:int, form_id:int } $atts + * @param array<int,string> $result Passed by reference. + * @global array $frm_vars + * @return void
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
.github/workflows/jscs.ymlis excluded by!**/*.yml
📒 Files selected for processing (1)
classes/controllers/FrmEntriesController.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/controllers/FrmEntriesController.php (1)
classes/models/FrmForm.php (2)
FrmForm(6-1269)get_current_form_id(1082-1091)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cypress
🔇 Additional comments (1)
classes/controllers/FrmEntriesController.php (1)
396-407: LGTM on reordering globals and init; just verify call order assumptionBringing
global $frm_vars;and the$form_id/$hidden/$iinit to the top reads cleaner and avoids any accidental late use. Functionally equivalent. One small caution: this relies on$frm_vars['cols']having been populated bymanage_columns()before this filter runs. That’s the normal WP List Table call order, but please smoke-test the Entries screen to confirm auto-hiding still kicks in when >11 columns exist (including subform/separate-value columns). If$frm_vars['cols']isn’t set, your fallback ($i = 0) safely no-ops the auto-hide, which is acceptable.
- Add 'run analysis' label condition to jscs.yml workflow - Follow same pattern as other workflows (cypress, phpunit, qadeploy) - ESLint now runs on push to master or when PR has 'run analysis' label - Enables manual control of when ESLint analysis runs on PRs
Fix all ESLint errors across JavaScript codebase
2 participants
This PR modernizes the admin JavaScript codebase by:
Key Benefits
formidable_admin.jsfile is now automatically minified and optimized through webpack's production build process