-
Notifications
You must be signed in to change notification settings - Fork 42
Wrappers refactor #832
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
Wrappers refactor #832
Conversation
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant and beneficial architectural refactoring. The reorganization of modules into feature-based subfolders and the removal of the wrappers directory cleans up the project structure. The introduction of barrel exports simplifies imports throughout the codebase.
My review focuses on a few areas to improve type safety and fix some issues that arose from the refactoring. I've identified some incorrect import paths that will cause compilation errors, and I've also suggested improvements to the new wrapper abstractions to make them more type-safe, which will reduce the need for unsafe type casts. Overall, this is a great improvement to the codebase.
f706ecd to
5b405c7
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This is an excellent and significant refactoring of the project's architecture. The move to a feature-based structure within src/modules, the removal of the src/wrappers directory, and the introduction of a clear class hierarchy for module wrappers greatly improve the codebase's scalability and maintainability. The addition of barrel files and the new Architecture.md documentation are also fantastic improvements.
My review includes a few suggestions:
- A critical fix for a type safety issue in the
onSubmitevent handler inAdyenCheckout.tsx. - Suggestions to improve code clarity and conciseness in
AdyenCheckout.tsx,ModuleWrapper.ts, andSessionWrapper.ts. - A minor formatting suggestion for the new architecture documentation.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant architectural refactoring, primarily reorganizing the module and wrapper structure for Adyen components. Key changes include the creation of a new docs/Architecture.md file detailing the updated design, a complete overhaul of the src/modules directory with new base wrapper classes (EventListenerWrapper, ModuleWrapper, PaymentComponentWrapper, ActionHandlingComponentWrapper), and dedicated wrappers for each Adyen component (e.g., ApplePayWrapper, DropInWrapper, GooglePayWrapper, InstantWrapper, ActionModuleWrapper, AdyenCSEWrapper, SessionWrapper).
Type definitions and constants have been consolidated and moved into src/core and src/modules/base/constants.ts respectively, with many old wrapper files being deleted. The AdyenCheckout.tsx component was updated to use the new wrapper hierarchy and isSupported event checking mechanism, replacing direct events.includes checks. The onBinLookuop event name was corrected to onBinLookup. The review comments highlight minor TypeScript compilation issues in test files related to accessing private/protected members of the new wrapper classes, suggesting explicit casting to any for testing purposes.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant architectural refactor, moving from direct NativeModules access to a new wrapper-based module system with a clear class hierarchy for event handling and component management. Key changes include the creation of EventListenerWrapper, ModuleWrapper, PaymentComponentWrapper, and ActionHandlingComponentWrapper classes, which centralize event subscription logic and isSupported checks. The AdyenCheckout component was updated to leverage this new wrapper system, replacing direct nativeComponent.events.includes() calls with nativeComponent.isSupported(). Many core types, constants, and utility functions were reorganized into barrel exports and new module-specific files, improving code structure and maintainability. Additionally, comprehensive architecture documentation was added. Review comments highlight concerns about unsafe type casting (as unknown as AdyenActionComponent) in the AdyenCheckout component's event handlers, suggesting a public API change for onSubmit to AdyenComponent for better type safety. Other comments point out that several new test cases for the wrapper classes access private/protected properties, recommending their removal to focus on public API testing.
|
johnayeni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!



Changes
Architecture Refactor
src/modules/into feature-based subfolderssrc/wrappers- Consolidated wrapper logic into respective module foldersTypo fix
onBinLookuopevent name was corrected toonBinLookupBarrel Exports
Added index.ts barrel files
Cleanup
src/core/AdyenCheckoutError.ts(unused)src/modules/cse/types.ts(co-located with CSE module)core/AdyenNativeModulestocore/typesIntroduce tests
Now
src/modulesare covered by tests.