Skip to content
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

[React] improve error handling in resolveReactComponent #2006

Open
wants to merge 3 commits into
base: 2.x
Choose a base branch
from

Conversation

teklakct
Copy link

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
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.`);

@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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
React Status: Needs Review Needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants