Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Correctly specify types for modern React contexts #10311

Closed
wants to merge 19 commits into from

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Mar 7, 2023

For https://github.com/vector-im/wat-internal/issues/65
Requires element-hq/element-web#24764


This change is marked as an internal change (Task), so will not be included in the changelog.

@t3chguy t3chguy added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Mar 7, 2023
@t3chguy t3chguy requested a review from a team as a code owner March 7, 2023 17:20
@t3chguy t3chguy mentioned this pull request Mar 7, 2023
@richvdh
Copy link
Member

richvdh commented Mar 8, 2023

clearing review req while in draft

Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

👏 Looks great!

@t3chguy
Copy link
Member Author

t3chguy commented Mar 13, 2023

babel/babel#13779 and microsoft/TypeScript#45995 are inherently blocking this if we want to use the declare syntax without disabling class properties and/or constructor property shorthand.

… t3chguy/react18/context

# Conflicts:
#	src/HtmlUtils.tsx
#	src/SlashCommands.tsx
#	src/Unread.ts
#	src/components/structures/SpaceRoomView.tsx
#	src/components/views/elements/AppTile.tsx
#	src/components/views/elements/PersistentApp.tsx
#	src/components/views/room_settings/AliasSettings.tsx
#	src/components/views/rooms/RoomList.tsx
#	src/components/views/rooms/RoomSublist.tsx
#	src/components/views/settings/DevicesPanel.tsx
#	src/components/views/settings/tabs/room/GeneralRoomSettingsTab.tsx
#	src/stores/room-list/SlidingRoomListStore.ts
#	test/stores/room-list/SlidingRoomListStore-test.ts
@Johennes
Copy link
Contributor

babel/babel#13779 and microsoft/TypeScript#45995 are inherently blocking this if we want to use the declare syntax without disabling class properties and/or constructor property shorthand.

It looks like these issues haven't seen meaningful updates since 2021. 😕

Possibly a stupid question but is there any alternative that would not make us block the React 18 update on them?

@t3chguy
Copy link
Member Author

t3chguy commented Aug 21, 2023

@Johennes we would either need to ditch class property initialization or typescript constructor properties as they do not play well together once you enable support for the declare syntax.

@Johennes
Copy link
Contributor

@Johennes we would either need to ditch class property initialization or typescript constructor properties as they do not play well together once you enable support for the declare syntax.

Hm, sounds like we couldn't realistically do either since both appear to be widely used? Is there a way in which we could avoid the declare syntax, assuming that's what's triggering the issue?

@t3chguy
Copy link
Member Author

t3chguy commented Aug 22, 2023

Is there a way in which we could avoid the declare syntax, assuming that's what's triggering the issue?

Maybe with a bunch of ts-ignores, but I would advise against that

@Johennes
Copy link
Contributor

babel/babel#13779 and microsoft/TypeScript#45995 are inherently blocking this if we want to use the declare syntax without disabling class properties and/or constructor property shorthand.

As mentioned in the chapter sync today, element-hq/element-web#26079 may or may not help unblock this. I haven't done any explorations into that direction so far though.

@dzienisz
Copy link

Can I help somehow with this? React 18 is a must today.

@t3chguy
Copy link
Member Author

t3chguy commented Jul 30, 2024

@dzienisz sure if you know of a way around babel/babel#13779 & microsoft/TypeScript#45995 whilst allowing the use of the declare syntax to enable use of React 18 contexts in class components

@dzienisz
Copy link

dzienisz commented Aug 1, 2024

My quick advice would be to ts-ignore it and write a comment with a link to this issue. Typescript is just a superset of JavaScript that, in my option it is to help you write a bug free code. In this case, Typescript has a bug, and it should not block you from upgrading this app.

@t3chguy
Copy link
Member Author

t3chguy commented Aug 2, 2024

@dzienisz even if you do that, you end with ~60+ broken tests due to changes in React & RTL. If you want to help you are more than welcome to fix the remaining ~30. #10314

@dzienisz
Copy link

dzienisz commented Aug 2, 2024

Sure, I will try to run the project and jump into it, or we can meet one, and you could make me a quick into. What do you think?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants