internalise private stuff, fixing dual types incompatibility #4842
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 2c67201:
|
|
Thanks for the help, let's see if it passes the CI 🤞 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4842 +/- ##
=======================================
Coverage 93.49% 93.49%
=======================================
Files 207 207
Lines 5015 5015
Branches 1368 1326 -42
=======================================
Hits 4689 4689
Misses 326 326
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -18,6 +18,7 @@ export class Value<T> { | |||
| */ | |||
| readonly hasToBeCloned: boolean; | |||
| /** | |||
| * @internal | |||
There was a problem hiding this comment.
This one seems to be on an unrelated field? 🤔
There was a problem hiding this comment.
any private field of an exported class, or symbol on an exported interface makes the types incompatible
|
|
|
Looks like all pass except doc publishing due to no permission |
|
Seems green ✅ I'll just checkout the resulting package tomorrow morning to see how it behaves |
| @@ -4,7 +4,9 @@ | |||
| * @public | |||
| */ | |||
| export class PreconditionFailure extends Error { | |||
| /** @internal */ | |||
| private static readonly SharedFootPrint: symbol = Symbol('fast-check/PreconditionFailure'); | |||
There was a problem hiding this comment.
Probably worth changing it into a Symbol.for so that we make sure that CJS and MJS world can interoperate between each others.
There was a problem hiding this comment.
sadly Symbol.for is also no type fix;
const a = Symbol.for("a")
const b = Symbol.for("a")
const c = (_: typeof b) => 1
c(a)
//^ Argument of type 'typeof a' is not assignable to parameter of type 'typeof b'.There was a problem hiding this comment.
Definitely! Not asking to drop the @internal, just asking to switch from Symbol to Symbol.for so that CJS entities could be consumed by MJS ones... And it will be better aligned with typings that just tell: don't care for symbols (for external users).
I'm wondering if this one would be an issue for you too: export const cloneMethod = Symbol('fast-check/cloneMethod');. I'm mostly thinking about export function hasCloneMethod<T>(instance: T | WithCloneMethod<T>): instance is WithCloneMethod<T>, if we apply your trick users will see a method like export function hasCloneMethod<T>(instance: T): instance is T and their code will refuse to access the symbol, no? 🤔
Well if you don't have to change cloneMethod then the problem right above does not exist 😄
Maybe I can deal with Symbol.for in a separate PR as it might breaks other things 🤔
There was a problem hiding this comment.
yea separate would be great.
the current implementation is tested and works in the @effect/schema case
There was a problem hiding this comment.
Ok perfect! If the current change is enough to patch all the problem you faced then we can probably go with it. I'll just run a last manual set of tests on it but it should be ok. And I'm still in between patch or minor, but patch seems ok as we mostly make the typings relaxed on something probably not used that much 🤔
Useless for now 😢 I hope to put it back once I can move back to full ESM. Hope that recent changes around CJS-MJS interrupt will help me to only ship ESM version in a few years... At that point in time I might re-enable that opaque type. For now it only helps me internally and may still be useful for users so that they reference the future opaque type in their typings (even if, nothing is opaque in it 😂). |
|
🚀 It's live on 3.17.1 |
fixes #4519, #4354