-
Notifications
You must be signed in to change notification settings - Fork 30
feat: add types to web implementation, extract common types #357
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
base: @maksymilianrojek/set-up-web-input
Are you sure you want to change the base?
feat: add types to web implementation, extract common types #357
Conversation
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.
Pull request overview
This PR enhances type safety by adding comprehensive TypeScript types to the web implementation and extracting common types shared between native and web platforms into a centralized location.
Key changes:
- Created
src/common/types.tsto house shared type definitions (OnChangeTextEvent,OnChangeHtmlEvent,OnChangeStateEvent,OnMentionDetected,OnChangeMentionEvent, andEnrichedTextInputInstanceBase) - Added complete type definitions to
src/web/EnrichedTextInput.tsxincluding props, events, and imperative handle methods - Fixed typo in function name from "Missconfigured" to "Misconfigured" in native implementation
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/common/types.ts | New file containing shared type definitions extracted from native implementation |
| src/native/EnrichedTextInputNativeComponent.ts | Updated to import shared types from common/types instead of defining them locally |
| src/native/EnrichedTextInput.tsx | Updated to import shared types from common/types and fixed typo in function name |
| src/web/EnrichedTextInput.tsx | Transformed from minimal stub to fully-typed implementation with complete type definitions and imperative handle methods |
| src/index.tsx | Added exports for common types to make them available to web consumers |
| src/index.native.tsx | Reorganized type exports to import common types from centralized location |
| apps/example-web/src/App.tsx | Updated to pass placeholder prop instead of hardcoding it in component |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…jek/common-ts-types-web
| OnLinkDetected, | ||
| OnMentionDetected, | ||
| OnChangeSelectionEvent, |
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.
Any reason why we essentially omit these two from the web editor? I think we're trying to achieve API parity, even if we use no-ops in some cases (which might be true for OnChangeSelectionEvent on web), I think we still want the API interface to be identical.
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.
On the web those two are in './web/EnrichedTextInput' file, so they are exported using export * from './web/EnrichedTextInput';. I see it now that it might be confusing
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.
Yeah, now I see native side is using codegen types in these. I wonder if the users will run into typing issues if they had a shared codebase for both web and native and there would be essentially two possible definitions for these interfaces..
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.
@maksymilianrojek any resolutions to that one? We can definitely proceed with the PR once this matter is dealt with.
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.
I've added common types to the types.ts, added handlers to the native side to "translate" native events to the common event type. c307304
…jek/common-ts-types-web
Summary
Changes:
Test Plan
Compatibility
This PR will be merged to a collective PR where all web-related changes will be kept until the implementation is complete.