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
Upgrade project to TypeScript #681
base: nfr/ts-major-refactor
Are you sure you want to change the base?
Upgrade project to TypeScript #681
Conversation
The explicit annotation isn't needed because the type is inferred by assignment.
Co-authored-by: Harjot Singh <contact@harjot.me>
Controller/ShabadInfo had issues fetching the shabad property from Line because an outdated Line import was used in the contexts file.
|
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.
Excellent work 🫡
A few catches, and be careful to consider changes that seem harmless (destructured variable default value assignments) that can actually affect the behaviour!
next: boolean, | ||
main: boolean, | ||
id: string, | ||
hotkey?: string | null, |
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.
The type here is undefined | string | null
- do we actually need the null
value? Coupled with the null
assignment on L57, that may be a change in semantics!
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.
This was directly translated from what was there before - defaultProps
assigned null
to hotkey
, and it also wasn't marked with isRequired
, hence the optional marker here. Would you like me to change this?
And also, this is something I encountered in .SHIFT, too. If I remove the optional property from hotkey
here, if you don't pass a value to hotkey
when creating the NavigatorLine
component, then it gives this error:
Property 'hotkey' is missing in type '{ focused: boolean; main: boolean; next: boolean; register: () => any; timestamp: any; QueryBuilderType: LinesBuilder; id: string; shabadId: string; sourcePage: number; sourceLine: number; ... 11 more ...; key: string; }' but required in type 'NavigatorLineProps'.ts(2741)
So with TypeScript, how do you factor in default values like this without making the param optional?
@@ -37,7 +37,7 @@ const ShabadInfo = () => { | |||
// Icon changes when open | |||
const barIcon = isPopoverOpen ? faTimesCircle : faInfoCircle | |||
|
|||
const { sourceId, writerId, section } = shabad || line.shabad | |||
const { sourceId, writerId, section } = shabad || line?.shabad || { sourceId: '', writerId: '', section: '' } |
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.
The additional || { sourceId ...}
would actually constitute a code change here! Are we sure this is what we intend? Is this just to counteract that shabad may not be present here? I think that is not a possible situation, and if that's the case, we can restructure it to
if (!shabad || !line?.shabad) {
console.error('Missing shabad')
return null
}
const { source, writer }...
the if
statement will narrow down the TS type
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 think the Line
, Shabad
, etc. types in the contract data.ts
file need updating, basically. So without this default assignment, this bit gives the error: Property 'sourceId' does not exist on type 'Shabads | undefined'.
so I'm guessing I added this to deal with that until we fixed the other types.
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.
So what should I do here? Should I remove this default assignment for now and then we'll update the contract
types in another task later?
@@ -39,12 +38,19 @@ import Navigator, { Bar as NavigatorBar } from './Navigator' | |||
import Search from './Search' | |||
import ToolbarButton from './ToolbarButton' | |||
|
|||
type OnHover = ( message: string | null ) => Record<string, any> | |||
|
|||
type TopBarProps = { |
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.
The old prop types for TopBar
had title
+ onHover
down as mandatory. Are the old prop types wrong?
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.
Looking at the old props now, I don't quite understand the logic. Why have defaultProps
set if it's mandatory to pass in values for them?
I think it makes sense now to make the props mandatory but remove the default values.
apps/frontend/src/Overlay/Line.tsx
Outdated
@@ -31,7 +36,7 @@ const Line = ( { | |||
{partitionLine( gurmukhi ) | |||
.map( ( line, lineIndex ) => ( | |||
<span key={lineIndex}> | |||
{line.map( ( { word, type }, i ) => <span key={`${word}-${type}-${i}`} className={classNames( type, 'word' )}>{word}</span> )} | |||
{line.map( ( { word, type }, i ) => <span key={`${word}-${type || ''}-${i}`} className={classNames( type, 'word' )}>{word}</span> )} |
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.
The || ''
seems a little odd here - certainly shouldn't do it to satisfy the type system. Surely partitionLine
should be typed to always return a shape of {word: string, type: string}[]
so there's no need for the || ''
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.
Okay so I've changed this back, which means the template string null error is back.
In classifyWord
function in line.ts
, look at this:
export const classifyWord = ( word: string, strip = true ) => ( { word: strip ? stripVishraams( word ) : word, type: Object .entries( vishraams ) // TODO: Do we want type to be null or a blank string? .reduce( ( type: string, [ pauseType, pauseChar ] ) => ( // Check if last char in word is the current pause char, and return that type if so word.slice( -1 ) === pauseChar ? pauseType : type ), null ), } )
Can I set the type value to be ''
instead of null
? That would fix the type (lol) issue.
) | ||
|
||
if (!connected) return null | ||
if ( !connected ) return null | ||
|
||
return ( | ||
<div className={classNames( { empty: !line }, 'overlay' )}> | ||
<ThemeLoader name={overlayName} /> | ||
|
||
<Line | ||
{...overlay} |
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.
simpleGraphics
has been removed here! How comes?
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.
The Line
props don't actually include it, and neither did Line.propTypes
, so I'm assuming that's why I removed it from the props here. It's apparently not used in the component. Did I miss something?
This will make this file consistent with the shared ThemeLoader file
The project currently only uses vanilla JavaScript and React.
This PR upgrades a lot of the project from JavaScript to TypeScript, which will improve the developer experience and make it easier to know what data types are used for all of the various data structures in the code.