-
Notifications
You must be signed in to change notification settings - Fork 943
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
fix: enable strict mode for TS types #1466
Conversation
Is there anything that needs to be done to move forward with this PR? I'm trying to upgrade our large app to RN 0.74 and that uses TypeScript 5, at which point there's no way out of type checking imported .ts dependencies. Because I'd be happy to help if we could get a version released that works with latest RN versions, given Flipper is removed in those versions Reactotron is more needed than ever. 😄 |
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.
There are also problems in:
- "src/stopwatch.ts":
typeof window !== "undefined" &&
=> Cannot find name 'window'.performanceNow(started)[1]
=> Element implicitly has an 'any' type because expression of type '1' can't be used to index type 'Number'.
- "src/validate.ts"
typeof host === "string" && host && host !== ""
=> Type 'string | boolean' is not assignable to type 'boolean'.!isHostValid(host)
=> Argument of type 'string | undefined' is not assignable to parameter of type 'string'. Type 'undefined' is not assignable to type 'string'.!isPortValid(port)
=> Argument of type 'number | undefined' is not assignable to parameter of type 'number'. Type 'undefined' is not assignable to type 'number'.!onCommandValid(onCommand)
=> Argument of type '((command: any) => void) | undefined' is not assignable to parameter of type '(cmd: string) => any'. Type 'undefined' is not assignable to type '(cmd: string) => any'.
@@ -60,7 +60,7 @@ function serialize(source, proxyHack = false) { | |||
* @param {*} value - The value to replace. | |||
*/ | |||
function serializer(replacer) { |
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.
Missing replacer
type
@@ -352,7 +355,7 @@ export class ReactotronImpl implements ReactotronCore { | |||
} | |||
|
|||
// this is ws style from require('ws') on node js | |||
if ("on" in socket && socket.on) { | |||
if ("on" in socket && socket.on!) { |
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.
Missing evt
type in line 370
Any movement on this PR? I'm seeing these TS errors 👀 @bruno-fialho @marcusnunes89
|
ready to merge? 👀 🚀 @marcusnunes89 |
@tabbathacrouch I don't have permission to merge, I'm just one of those who want this fixed so I can use it :P |
Thank you for this addition! Sorry this sat for so long. Our open source labor at Infinite Red relies on contributions from our team members during downtime between client projects and donations of our free time, so sometimes painful issues like this fall through the cracks. We needed to do a little more work to get our |
Is there a way to use or do we need to wait for an update to |
Please verify the following:
yarn build-and-test:local
passesREADME.md
(or relevant documentation) has been updated with your changesDescribe your PR
This PR enables strict mode for the
reactotron-core-client
codebase to fix issues documented here:#1430