feat(builder): add cacheStrategy option
#520
Conversation
rafaelss95
force-pushed
the
feat/builder-cache-strategy
branch
2 times, most recently
from
64aee5b
to
46c96a9
Compare
Nx Cloud ReportCI ran the following commands for commit fabd4ee. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch Sent with 💌 from NxCloud. |
| @@ -10,6 +10,7 @@ export interface Schema extends JsonObject { | |||
| fix: boolean; | |||
| cache: boolean; | |||
| cacheLocation: string | null; | |||
| cacheStrategy: 'content' | 'metadata' | null; | |||
There was a problem hiding this comment.
Shouldn't null be undefined here? Same as cacheLocation above.
There was a problem hiding this comment.
The problem is that the Schema extends JsonObject, which doesn't accept undefined.
There was a problem hiding this comment.
Can optional property ?: be used?
There was a problem hiding this comment.
No, because it has the same effect of | undefined.
There was a problem hiding this comment.
I mean add it into JsonObject.
| fix: !!options.fix, | ||
| cache: !!options.cache, | ||
| cacheLocation: options.cacheLocation || undefined, | ||
| ignorePath: options.ignorePath ?? undefined, |
There was a problem hiding this comment.
Why change these to use ?? instead of ||?
|| was already doing everything we needed it to, and now technically with your change 0, NaN and '' will be set preferentially to undefined for these values, which is not desired. It may be unlikely that those values make their way through the code paths here, but there needs to be an objective reason to make this change in the first place IMO, and it seems objectively less desirable to me.
There was a problem hiding this comment.
You're right. I'll revert this 😃
rafaelss95
force-pushed
the
feat/builder-cache-strategy
branch
6 times, most recently
from
2217a4f
to
2fedb53
Compare
rafaelss95
force-pushed
the
feat/builder-cache-strategy
branch
from
2fedb53
to
b5f4c62
Compare
Codecov Report
@@ Coverage Diff @@
## master #520 +/- ##
==========================================
+ Coverage 70.39% 71.86% +1.47%
==========================================
Files 17 15 -2
Lines 689 654 -35
Branches 141 131 -10
==========================================
- Hits 485 470 -15
+ Misses 139 120 -19
+ Partials 65 64 -1
|
|
Thanks @rafaelss95! |
Closes #410.
Waiting for DefinitelyTyped/DefinitelyTyped#53485.