Skip to content

[React] Improve error handling in resolveReactComponent #2006

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 23, 2025

Conversation

teklakct
Copy link
Contributor

Q A
Bug fix? no
New feature? no
Issues
License MIT

WHAT

Improve error handling and readability in window.resolveReactComponent function

  • Enhance error messages for clarity:
    • Indicate when a module exists but lacks a default export.
    • (still) List available controllers when a specified controller does not exist.

With those changes, when resolving a module with no default export, an error message with a hint will be shown:

React controller "${name}" could not be resolved. Ensure the module exports the controller as default.

WHY

I found that when someone creates a module without a default export then the error message is quite confusing.
For example:

// NoDefaultExportComponent.jsx
export const SomeRandomName = () => {
    return <div>Hello</div>;
}

Whenever there will be an element like this

<div data-controller="NoDefaultExportComponent">
   ...
</div>

An error like below in console appear:

Error connecting controller

React controller "NoDefaultExportComponent" does not exist. Possible values: SomeComponent, AnotherComponent, NoDefaultExportComponent

Which is not exactly true. The module NoDefaultExportComponent was found but there is no required default export.

@carsonbot carsonbot added React Status: Needs Review Needs to be reviewed labels Jul 24, 2024
@teklakct teklakct force-pushed the feature/resolve_react_error_message branch from 53f7119 to a312fee Compare July 24, 2024 19:30
@teklakct
Copy link
Contributor Author

Alternatively function importAllReactComponents may omit every module without default export. Something like this

    const importAllReactComponents = (r: __WebpackModuleApi.RequireContext) => {
-        r.keys().forEach((key) => (reactControllers[key] = r(key).default));
+        r.keys().forEach((key) => {
+            if (r(key).default !== undefined) {
+                reactControllers[key] = r(key).default;
+            }
+        });
    };

However, with these changes, it will be more difficult to determine if a module or default export is missing.

Comment on lines 42 to 44
throw new Error(
`React controller "${name}" could not be resolved. Ensure the module exports the controller as default.`
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new Error(
`React controller "${name}" could not be resolved. Ensure the module exports the controller as default.`
);
throw new Error(`React controller "${name}" could not be resolved. Ensure the module exports the controller as default.`);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran biome check and applied the suggested formatter changes

@smnandre
Copy link
Member

Hi @teklakct ! Thanks for this

Could you rebuild the JS dist files ? (that's the reason of the CI failure)

From the root of the monorepo: yarn run build

(you may need to rebase before)

@Kocal Kocal added Status: Needs Work Additional work is needed and removed Status: Needs Review Needs to be reviewed labels Mar 4, 2025
@teklakct teklakct force-pushed the feature/resolve_react_error_message branch from bb23f4a to b7dba4b Compare March 8, 2025 23:07
@carsonbot carsonbot added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Mar 8, 2025
Copy link
Contributor

github-actions bot commented Mar 8, 2025

📊 Packages dist files size difference

Thanks for the PR! Here is the difference in size of the packages dist files between the base branch and the PR.
Please review the changes and make sure they are expected.

FileBefore (Size / Gzip)After (Size / Gzip)
React
register_controller.js 827 B / 398 B 1.03 kB+28% 📈 / 466 B+17% 📈

@teklakct
Copy link
Contributor Author

teklakct commented Mar 8, 2025

@smnandre sorry for the delay. I must have missed the notification earlier.

@smnandre
Copy link
Member

smnandre commented Mar 9, 2025

@teklakct don't be :) I also lost tracking of this PR to be honest!

Seems good to me !

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Mar 9, 2025
@smnandre smnandre requested a review from Kocal March 9, 2025 02:02
@carsonbot carsonbot added Status: Needs Review Needs to be reviewed and removed Status: Reviewed Has been reviewed by a maintainer labels Mar 9, 2025
Copy link
Member

@Kocal Kocal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this, and sorry for the delay.

Do you think we could do the same for UX Vue and UX Svelte?

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Mar 12, 2025
@Kocal Kocal changed the title [React] improve error handling in resolveReactComponent [React] Improve error handling in resolveReactComponent May 23, 2025
@Kocal Kocal force-pushed the feature/resolve_react_error_message branch from b7dba4b to 9e36018 Compare May 23, 2025 09:16
@Kocal
Copy link
Member

Kocal commented May 23, 2025

Thank you @teklakct.

@Kocal Kocal merged commit 819f66e into symfony:2.x May 23, 2025
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
React Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants