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.

Sign up for GitHub

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

Jump to bottom

Refactor multiple old files to TypeScript #1010

Merged
merged 8 commits into from
Nov 17, 2023
Merged

Refactor multiple old files to TypeScript #1010

merged 8 commits into from
Nov 17, 2023

Conversation

acezard
Copy link
Contributor

@acezard acezard commented Nov 10, 2023 •
edited
Loading

Key Changes:

  1. TypeScript Integration:

    • Implemented typings across numerous previously untyped files (8b112609), ensuring a more robust and error-resistant codebase.
    • Added typings to most of the functions (50b5ed73), providing clearer interfaces and improving developer experience.

  2. File Renaming and Reorganization:

    • Renamed several files to .ts (8ef5488d, a2ff6330), aligning with TypeScript conventions and improving code consistency.
    • Converted relevant .ts files to .tsx where JSX is present (fbe4db2a), ensuring proper file type representation and IDE support.

  3. Cleanup and Refinement:

    • Removed redundant '.js' imports (63a75e73), streamlining the code and reducing confusion around module systems.

@acezard acezard changed the title refactor: rename rootNavigation to .ts Refactor multiple files in libs to full typings Nov 10, 2023
@acezard acezard force-pushed the refactor--migrate-libs-files-to-typescript branch from 9ca7183 to 2fad88f Compare November 11, 2023 09:55
@acezard acezard marked this pull request as ready for review November 11, 2023 09:55
@acezard acezard force-pushed the refactor--migrate-libs-files-to-typescript branch 2 times, most recently from 04f1438 to 50b5ed7 Compare November 11, 2023 13:41
@acezard acezard changed the title Refactor multiple files in libs to full typings Refactor multiple old files in to TypeScript Nov 11, 2023
@acezard acezard force-pushed the refactor--migrate-libs-files-to-typescript branch 2 times, most recently from 36ba60f to 789e91e Compare November 12, 2023 14:52
@acezard acezard changed the title Refactor multiple old files in to TypeScript Refactor multiple old files to TypeScript Nov 12, 2023
zatteo
zatteo reviewed Nov 14, 2023
View reviewed changes
src/app/domain/geolocation/helpers/index.ts
export const parseISOString = ISOString => {
let b = ISOString.split(/\D+/)
export const parseISOString = (ISOString: string): Date => {
const b = ISOString.split(/\D+/).map(Number)
return new Date(Date.UTC(b[0], --b[1], b[2], b[3], b[4], b[5], b[6]))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@paultranvan is it okay for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what it means for CozyGPSMemory: will it require some extra ts config?
I'm a bit worried that it may introduce more harm than good

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'm not sure what it means for CozyGPSMemory: will it require some extra ts config? I'm a bit worried that it may introduce more harm than good

I don't think it should impact CozyGPSMemory, I think the issue here could be an unexpected behaviour at runtime with the casting to Number explicitly (this was required for stronger type inference).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the issue here could be an unexpected behaviour at runtime with the casting to Number explicitly

I'm not sure what you imply here: do you mean an issue could arise if the variable is actually not a number (which is normal), or that this issue could arise anyway?

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 removed runtime modifications

Ldoppea
Ldoppea approved these changes Nov 15, 2023
View reviewed changes
Copy link
Member

@Ldoppea Ldoppea left a comment

Choose a reason for hiding this comment

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

Nice one.

Only the comment about makeMetadata is important, other ones are just nit and double-checks

src/libs/functions/getHostname.ts
!('url' in nativeEvent) ||
typeof nativeEvent.url !== 'string'
)
return
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we throw instead? Can this happen?

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 don't feel safe in throwing an error here as I don't know if the caller will expect it (fearing regressions). Logging a message instead then

src/components/webviews/jsInteractions/jsCozyInjection.ts Outdated
if (!window.cozy) window.cozy = {}
window.cozy.isFlagshipApp = true
window.cozy.ClientKonnectorLauncher = 'react-native'
window.cozy.ClientConnectorLauncher = 'react-native' // deprecated
window.cozy.flagship = ${makeMetadata(routeName)}
window.cozy.isSecureProtocol = ${isSecureProtocol || 'false'}
window.cozy.flagship = ${routeName ? makeMetadata(routeName) : ''}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe whe should instead

  window.cozy.flagship = ${makeMetadata(routeName ?? '')}

Seems like having an empty flagship field would produce more side effect than having a wrong immersive data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

src/components/webviews/jsInteractions/jsCozyInjection.ts Outdated
window.cozy.flagship = ${makeMetadata(routeName)}
window.cozy.isSecureProtocol = ${isSecureProtocol || 'false'}
window.cozy.flagship = ${routeName ? makeMetadata(routeName) : ''}
window.cozy.isSecureProtocol = ${isSecureProtocol ? 'true' : 'false'}
Copy link
Member

Choose a reason for hiding this comment

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

This isSecureProtocol ? 'true' : 'false' seems weird

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 agree but TS doesn't like outputting non-string values in template literals in this case

src/components/webviews/jsInteractions/jsLogInterception.spec.tsx Outdated
nativeEvent: {
data: string
}
} => ({
Copy link
Member

Choose a reason for hiding this comment

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

Those types should definitively be extracted somewhere else. Inlining them make the code very difficult to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

src/components/webviews/jsInteractions/jsLogInterception.ts
const dataPayload = JSON.parse(rawData) as {
type: string
data?: { type: string; log: string[] }
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: would be more correct to remove this as and to convert the next if to a typeguard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the issue is JSON methods all return any, have to cast them

@acezard
refactor: rename files to TypeScript
84f6944
@acezard
refactor: remove imports as ".js" files
bcbd784 
It makes it harder to rename files to ts as they won't be resolved
@acezard
refactor: add typings to most of the functions
6427b0d 
Some runtime logic had to be updated,
mostly adding more strict checks to avoid errors
@acezard
refactor: rename more files to TS
e7f55ed
@acezard
refactor: rename ts files to tsx where needed
efd9f8c
@acezard
refactor: implement typings to old files
5aca8e0 
These files are still used. Removed one unused file
@acezard acezard force-pushed the refactor--migrate-libs-files-to-typescript branch from 789e91e to 5aca8e0 Compare November 17, 2023 14:35
@acezard
refactor: Implement TS refactor PR comments
555da4f
@acezard
fix: temporarily fix ci issues
10e0a4e 
Can't fix them right now.
Will do after the big refactor branch is merged
@acezard acezard force-pushed the refactor--migrate-libs-files-to-typescript branch from fb50407 to 10e0a4e Compare November 17, 2023 14:56
@acezard acezard merged commit 9ae2c4a into master Nov 17, 2023
@acezard acezard deleted the refactor--migrate-libs-files-to-typescript branch November 17, 2023 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ldoppea Ldoppea Ldoppea approved these changes

@zatteo zatteo Awaiting requested review from zatteo zatteo is a code owner

@paultranvan paultranvan Awaiting requested review from paultranvan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@acezard @Ldoppea @paultranvan @zatteo