Package update: dont upload unmodified files aka dedupe #2080
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2080 +/- ##
==========================================
- Coverage 46.29% 46.22% -0.08%
==========================================
Files 408 408
Lines 19475 19510 +35
Branches 2258 2266 +8
==========================================
+ Hits 9016 9018 +2
- Misses 9449 9514 +65
+ Partials 1010 978 -32
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
nl0
force-pushed
the
pkg-update-dedupe
branch
2 times, most recently
from
19a18dd to
5d67c76
Compare
nl0
force-pushed
the
pkg-update-dedupe
branch
from
39987d6 to
3acb365
Compare
nl0
changed the title
There was a problem hiding this comment.
I checked the feature in the browser, didn't spot any bug. There is a control "Undo changes ←" when all files are the same (hashed), but I think, it's ok.
Code is good, just advice: I think that the better strategy for typescript migration is converting small modules to typescript instead of large ones. If you don't have small modules, you can create them by splitting large into small ones. Also, as you are using .js imports you can create intermediate types (PackageDialog/types.ts). Otherwise, you are forced to use any and typecasting.
In short: having a small number of tiny modules completely converted to typescript is better, than large ones converted partially with any and typecasting.
| const existingEntries = Object.entries(existing).map( | ||
| ([path, { isDir, size, hash }]) => { | ||
| if (path in deleted) { | ||
| return { type: 'deleted' as const, path, size } |
There was a problem hiding this comment.
'deleted' as const
I think you should use Enum for type
There was a problem hiding this comment.
why? const strings make more sense imo, and you can also use them as keys (like, for classes or some matches), and it's properly type-checked anyways
There was a problem hiding this comment.
You can use enum as object key too
enum State {
Deleted,
}
entry[State.Deleted]There was a problem hiding this comment.
yes, but it wont help with classes (or classes will have some number keys, which doesnt seem nice). we could use string enums, but they require more boilerplate, so, in the end, i dont see what we gain from that
There was a problem hiding this comment.
Profit in letting typescript already know type without manual casting as const
| Success: (v: { name: string; hash: string }) => v, | ||
| }, | ||
| ) | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-redeclare | ||
| type DialogState = tagged.InstanceOf<typeof DialogState> |
There was a problem hiding this comment.
I don't get it. Why not just pick another name?
There was a problem hiding this comment.
first, in this case it's getting imported together with the value, so no need for another import;
second, it's like an instance of a class, like, you usually dont create smth like type SomeClassInstance and use it like (x: SomeClassInstance) => whatever, you just use it directly like (x: SomeClass) => whatever;
so it's just a convenience / developer ergonomics thing
well, it requires some extra logic for comparing trees (maybe overriding final-form's equality check, not sure), and i guess it doesnt worth it right now
I agree on that, but I decided to migrate the modules I was modifying and directly importing, bc it was getting hard to reason about that code without proper contracts / typings for state and interfaces |
No description provided.