-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
General purpose memoization tool #43868
base: main
Are you sure you want to change the base?
General purpose memoization tool #43868
Conversation
Current state of the types in the Here is a TS Playground with minimal reproducible sample for the issue. Also here is a link to SO where the difficulty is discussed, but it is not 100% equivalent. In general, the If you know how to resolve the typings issue, please let me know 🙏 |
Video of the issue: mobile-web-issue.mov |
I completed the checklist pending mobile web screencasts, issues with my setup. |
@mananjadhav We have just changed the live-markdown package version in main so maybe you need to do clean npm install. Creating an adhoc build though |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
I tried reinstall with npm i, it didn't work. Anyway thanks for the adhoc build, will test now. |
@mountiny Tested and uploaded videos for the pending platforms (mobile web Chrome and mobile web Safari). |
50b98d8
Hi folks, thanks for the review guys! Recent changes consist:
When it comes to mobile web issues with sending a message, I've checked ad hoc builds and it worked fine. @mountiny @mananjadhav does the issue still exist? message-not-sending-mweb.movLet me know if we are good to go, thanks! 👍 PS There is a |
@kacper-mikolajczak Suggested a structure here
I tested on adhoc and it worked fine. Not sure something wrong with my local setup maybe. |
@kacper-mikolajczak The error seems to be on patch-package, @mountiny could you retry the job? |
We are seeing that fail on other PRs too, Fabio is looking right now |
Hi @mountiny! How is |
@kacper-mikolajczak this is fixed in main so you can merge main and it should be resolved |
@kacper-mikolajczak the lint seems to be failing now |
@mountiny Yup, there's a new use case (added just 2 days ago) about |
Will review once the change has been pushed. |
Current to do list:
Items below The [Fun TS fact] Here is a code for truncating the tuples: // Based on: https://stackoverflow.com/questions/67605122/obtain-a-slice-of-a-typescript-parameters-tuple
type TupleSplit<T, N extends number, O extends readonly any[] = readonly []> = O['length'] extends N
? [O, T]
: T extends readonly [infer F, ...infer R]
? TupleSplit<readonly [...R], N, readonly [...O, F]>
: [O, T];
type TakeFirst<T extends readonly any[], N extends number = T['length']> = TupleSplit<T, N>[0]; Now I need to connect it into generic options with |
Update: Needed to focus on other task for today, so no progress. I will be OOO Wednesday and Thursday, so let's get back to it on Friday! |
Details
PR implements POC2 (continuation of #43538) of memoization tool for general purpose use. The main goal is to:
Minor to do:
Fixed Issues
$ #42200
PROPOSAL: https://expensify.slack.com/archives/C01GTK53T8Q/p1716371827915779
Tests
LHN
FAB
Submit Expense
Profile
screenOffline tests
N/A
QA Steps
LHN
FAB
Submit Expense
Profile
screenPR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android.mp4
Android: mWeb Chrome
android-web.mp4
iOS: Native
ios.mov
iOS: mWeb Safari
ios-web.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4