[refactor] Migrate Discover schedules to tsx #1946

muhsinkamil · 2023-11-21T10:38:44Z

Relates to #1483

Migrate Discover schedules component to ts
Enrich types to the endpoint schedules/discover

netlify · 2023-11-21T10:38:49Z

✅ Deploy Preview for actualbudget ready!

Name	Link
🔨 Latest commit	`8eeb6e1`
🔍 Latest deploy log	https://app.netlify.com/sites/actualbudget/deploys/6562e9bee45fc70008ea7603
😎 Deploy Preview	https://deploy-preview-1946.demo.actualbudget.org
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

github-actions · 2023-11-21T10:40:46Z

Bundle Stats — desktop-client

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count	Total bundle size	% Changed
16	2.98 MB → 2.98 MB (-1 B)	-0.00%

Changeset

File	Δ	Size
`src/components/schedules/DiscoverSchedules.tsx`	🆕 +8.26 kB	0 B → 8.26 kB
`../loot-core/src/client/reducers/budgets.ts`	📉 -66 B (-1.36%)	4.75 kB → 4.69 kB
`src/components/schedules/DiscoverSchedules.js`	🔥 -8.22 kB (-100%)	8.22 kB → 0 B

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

No assets were bigger

Smaller

Asset	File Size	% Changed
static/js/main.js	1.16 MB → 1.16 MB (-1 B)	-0.00%

Unchanged

Asset	File Size	% Changed
static/js/665.chunk.js	824.28 kB	0%
static/media/Inter-italic.var.woff2	239.29 kB	0%
static/media/Inter-roman.var.woff2	221.86 kB	0%
static/js/444.chunk.js	156.11 kB	0%
static/js/wide-components.chunk.js	119.83 kB	0%
static/js/231.chunk.js	117.37 kB	0%
static/js/reports.chunk.js	74.89 kB	0%
static/js/narrow-components.chunk.js	52.88 kB	0%
static/js/301.chunk.js	13.31 kB	0%
static/js/473.chunk.js	13 kB	0%
static/js/resize-observer-polyfill.chunk.js	8.88 kB	0%
static/css/main.css	7.41 kB	0%
asset-manifest.json	2.07 kB	0%
index.html	1.66 kB	0%
static/media/browser-server.js	911 B	0%

github-actions · 2023-11-21T10:40:47Z

Bundle Stats — loot-core

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count	Total bundle size	% Changed
2	2.22 MB → 2.22 MB (+2 B)	+0.00%

Changeset

File	Δ	Size
`packages/loot-core/src/server/schedules/find-schedules.ts`	📈 +2 B (+0.02%)	11.43 kB → 11.43 kB

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset	File Size	% Changed
kcab.worker.js	1.23 MB → 1.23 MB (+2 B)	+0.00%

Smaller

No assets were smaller

Unchanged

Asset	File Size	% Changed
xfo.xfo.kcab.worker.js	1014.46 kB	0%

MatissJanis

👋 Exciting to see the TypeScript migration coming along. Thanks for your work on this! I pointed out some things that we should work on fixing, but apart from that LGTM!

packages/desktop-client/src/components/schedules/DiscoverSchedules.tsx

packages/loot-core/src/server/schedules/find-schedules.ts

MatissJanis · 2023-11-21T18:35:17Z

packages/loot-core/src/types/models/schedule.d.ts

+  payee: PayeeEntity['id'];
+  date: ScheduleEntity['_date'];
+  amount: ScheduleEntity['_amount'];
+  _conditions: [


🔨 warning: ‏this should use enum type instead of an array with specific values (_conditions: Array<A | B | C>)

https://www.typescriptlang.org/play?#code/JYOwLgpgTgZghgYwgAgCoQM5mQbwFDKHIBucANgK4QBcyA2gUUzsgPYAOtA5MBlwNzIYwCGQAm3RAlYVwA5AF8ANIyaEWHbr3nDRE5F3ZwAnhAjzlq5AF08CvHmkgsyMXDBxa6FwF5cV0koaeitmNk4DbSUhEXFuI1NzRStbBSA

I think, it should be an array of values instead, because these values are hard-coded in the same order in the return of find-schedules and if we change to enum, we get error in there that the source has 4 elements but target allows only 1

Currently it's 4 objects, but this might change to more or less in the future. We shouldn't tie the typings to the actual business logic.

Eventually I think we'll even get rid of this Conditions typing and instead use a generic one across the entire product. But it'll take us some time to get there.. :)

shouldn't tie the typings to the actual business logic

Agree !!, however, doing it in (_conditions: Array<A | B | C>) is not working here, what other ways would you suggest here 🤔 ? or should we keep it as Array<A | B | C> for now to change it in future ?

Here you go: https://github.com/actualbudget/actual/pull/1946/files#r1404613308

MatissJanis · 2023-11-24T19:31:56Z

packages/loot-core/src/types/models/schedule.d.ts

+    { op: 'is'; field: 'account'; value: AccountEntity['id'] },
+    { op: 'is'; field: 'payee'; value: PayeeEntity['id'] },
+    {
+      op: 'is' | 'isapprox';
+      field: 'date';
+      value: ScheduleEntity['_date'];
+    },
+    {
+      op: 'is' | 'isapprox';
+      field: 'amount';
+      value: ScheduleEntity['_amount'];
+    },
+  ];


Suggested change

{ op: 'is'; field: 'account'; value: AccountEntity['id'] },

{ op: 'is'; field: 'payee'; value: PayeeEntity['id'] },

{

op: 'is' | 'isapprox';

field: 'date';

value: ScheduleEntity['_date'];

},

{

op: 'is' | 'isapprox';

field: 'amount';

value: ScheduleEntity['_amount'];

},

];

_conditions: Array<

| { op: 'is'; field: 'account'; value: AccountEntity['id'] }

| { op: 'is'; field: 'payee'; value: PayeeEntity['id'] }

| {

op: 'is' | 'isapprox';

field: 'date';

value: ScheduleEntity['_date'];

}

| {

op: 'is' | 'isapprox';

field: 'amount';

value: ScheduleEntity['_amount'];

}

>;

Thank you !

You should use Array instead of [] here. I'm quite surprised it doesn't fail in CI..

But here's a playground so you can see the difference between the two types: https://www.typescriptlang.org/play?#code/C4TwDgpgBAKgFgSwM4CEBOECGBrJUC8UA3gFBTlQBumANgK4QBcUA2mRRwD7FQD2YzAOTJBAbigAzBBBoATIZgDGi3nQB2wMVAC+7DuW5E+AqMKRapM+abCYQECFt0cAuiV0lQkWIiQB1XjRcAmI9KloGZgBBNDQ7AB4wrh5+IRFxSzkFZVUNJySKQ2M08wzpLJs7B3yOAD53EhIVNSRgKGBfAH0AdwQaGh7A7GZ4ZACgvEJSDmp6JlYC8iNU0xEAGkly60Fbe0cdNcWUkzNBDcztpRV1TQOj5ZP1zashXeq71wam3ha2juQen0BgAjDA4Ea+dBYYJTMKzSILfQUB4lM7PCo7Kr7bSHJFLYqrcznLbZa55D54lGEtEXV5YwQU8hubRAA

yeah !, My bad !!, Thank you :) 👍

MatissJanis

Thanks so much!

trafico-bot bot added the 🔍 Ready for Review label Nov 21, 2023

MatissJanis requested changes Nov 21, 2023

View reviewed changes

trafico-bot bot added ⚠️ Changes requested Pull Request needs changes before it can be reviewed again 🔍 Ready for Review and removed 🔍 Ready for Review ⚠️ Changes requested Pull Request needs changes before it can be reviewed again labels Nov 21, 2023

muhsinkamil added 7 commits November 22, 2023 12:59

feat: add types for discover-schedule endpoint

094aa1c

refactor: move discover schedule component to tsx

c5ff081

refactor: change type for useSelected event & remove log

4c08f49

refactor(types): move entity def to d.ts file

718f84c

revert: remove the unwanted filter used for logging to useSelected

11377aa

chore: add release notes for the change

a00c7e4

refactor: provide type in useSendPlatformRequest to infer in usages

ecad163

muhsinkamil force-pushed the refactor/1483-discover-schedules branch from d9b8c03 to ecad163 Compare November 22, 2023 07:29

refactor: remove explicit type in runQuery

ae9f895

muhsinkamil requested a review from MatissJanis November 22, 2023 12:51

Merge branch 'master' into refactor/1483-discover-schedules

298e438

MatissJanis reviewed Nov 24, 2023

View reviewed changes

muhsinkamil and others added 3 commits November 25, 2023 21:34

refactor: provide or type in schedule conditions

b5aaa0e

Merge branch 'master' into refactor/1483-discover-schedules

77e05bc

refactor: provide array type to schedule conditions

8eeb6e1

MatissJanis approved these changes Nov 26, 2023

View reviewed changes

trafico-bot bot added ✅ Approved and removed 🔍 Ready for Review labels Nov 26, 2023

MatissJanis merged commit c3a9b4d into actualbudget:master Nov 26, 2023
18 of 19 checks passed

trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved labels Nov 26, 2023

FlorianLang06 pushed a commit to FlorianLang06/actual that referenced this pull request Mar 7, 2024

[refactor] Migrate Discover schedules to tsx (actualbudget#1946)

9f487f4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[refactor] Migrate Discover schedules to tsx #1946

[refactor] Migrate Discover schedules to tsx #1946

muhsinkamil commented Nov 21, 2023

netlify bot commented Nov 21, 2023 •

edited

Loading

github-actions bot commented Nov 21, 2023 •

edited

Loading

github-actions bot commented Nov 21, 2023 •

edited

Loading

MatissJanis left a comment

MatissJanis Nov 21, 2023

muhsinkamil Nov 22, 2023

MatissJanis Nov 22, 2023

muhsinkamil Nov 24, 2023

MatissJanis Nov 24, 2023

MatissJanis Nov 24, 2023

muhsinkamil Nov 25, 2023

MatissJanis Nov 25, 2023 •

edited

Loading

muhsinkamil Nov 26, 2023

MatissJanis left a comment

[refactor] Migrate Discover schedules to tsx #1946

[refactor] Migrate Discover schedules to tsx #1946

Conversation

muhsinkamil commented Nov 21, 2023

netlify bot commented Nov 21, 2023 • edited Loading

✅ Deploy Preview for actualbudget ready!

github-actions bot commented Nov 21, 2023 • edited Loading

Bundle Stats — desktop-client

github-actions bot commented Nov 21, 2023 • edited Loading

Bundle Stats — loot-core

MatissJanis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatissJanis Nov 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatissJanis left a comment

Choose a reason for hiding this comment

netlify bot commented Nov 21, 2023 •

edited

Loading

github-actions bot commented Nov 21, 2023 •

edited

Loading

github-actions bot commented Nov 21, 2023 •

edited

Loading

MatissJanis Nov 25, 2023 •

edited

Loading