fix(vanilla): Improve Snapshot type so that it respects builtin objects

[octet-stream]

Summary

This PR improves Snapshot type so that it does not affect builtin object, like Date.

Examples for old and new behaviours can be found in TS playground: for TS 3.5.1 and for TS 4.9.5. As you can see from the OldSnapshot example, it iterates through every member of Date, Map and Set object and converts these objects to readonly POJOs for some reason.

The solution might not be perfect though, so I would suggest adding some tests for typings (or maybe you already have and I just won't be able to find any, so please let me know).

Also, I wouldn't be able to run tests, because this project is hardly relying on Yarn, so will see if the tests are passing.

And lastly, I would like to ask if you can expose both Options (for useSnapshot hook) and Snapshot<T> types, so that people might implement extensions upon this hook and rely on public API.

Check List

• [x] yarn run prettier for formatting code and docs

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
valtio	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 18, 2023 at 1:09PM (UTC)

[codesandbox-ci[bot]]

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 092bec4d353db7f16f9202285255a83cbe0ec0ac:

Sandbox	Source
React	Configuration
React Typescript	Configuration
React Browserify	Configuration
React Snowpack	Configuration
React Parcel	Configuration

[dai-shi]

Thanks for your suggestions!

so I would suggest adding some tests for typings

Would you be able to help on it?

this project is hardly relying on Yarn

That's true, but I think you can run normal tests with npm test. (CI tests are really complicated.)

if you can expose both Options (for useSnapshot hook) and Snapshot types,

As you might guess, the first one can be extracted fairly easily, and the second one is intentional.

people might implement extensions upon this hook and rely on public API.

If they rely on internal types, I'd suggest to pin the valtio version and use it. We may really change them in the future without notice.

[octet-stream]

Would you be able to help on it?

Yup, I think I can write bunch of tests for Snapshot type using something like tsd. But I am have no idea how to get this done for patches for TS 3.4

That's true, but I think you can run normal tests with npm test. (CI tests are really complicated.)

Nope, it doesn't work and I have to install Yarn. And maybe I will, it's just that I have to do this for such small thing. Not a big problem, I think. But having scripts running using npm might be more convenient, as it is the default (and I personally could've run tests via pnpm).

As you might guess, the first one can be extracted fairly easily, and the second one is intentional.

And so I did, yes, but it would be just much more handful to have useSnapshot Options to be part of the public API.

If they rely on internal types, I'd suggest to pin the valtio version and use it. We may really change them in the future without notice.

Alright then :) Besides, for me, it's just a matter of typing the result of hook that wraps useSnapsoht to create a snapshot from useContext result.

[dai-shi]

Would you be able to help on it?

Yup, I think I can write bunch of tests for Snapshot type using something like tsd. But I am have no idea how to get this done for patches for TS 3.4

We can simply drop such tests for very old TS. Please make it a new test file.

That's true, but I think you can run normal tests with npm test. (CI tests are really complicated.)

Nope, it doesn't work and I have to install Yarn. And maybe I will, it's just that I have to do this for such small thing. Not a big problem, I think. But having scripts running using npm might be more convenient, as it is the default (and I personally could've run tests via pnpm).

Oops, my bad. My recent change requires yarn for test. Yeah, we may consider improving those build/test scripts in the future and I hope to make it work with npm too.

[octet-stream]

Added some tests for Snapshot type, ended up using ts-expect. They will be run through regular tsc --noEmit command.

[octet-stream]

Feels like we never reach an ideal solution. false positives are better than false negatives.

Can't agree with you more. So I've added SnapshotIgonre union type with the list of objects that Snapshot will not turn into readonly. Please let me know if I've missed something.

[octet-stream]

Out of curiosity: Why GitHub does not let me to request review from both of you at the same time? x)

[octet-stream]

Wellp, looks like I need yarn after all :)

[octet-stream]

Wait, which yarn version do I need? Is it 1.x?

[dai-shi]

yes, it's classic yarn.

[octet-stream]

Okay, I have no idea why tests for old ts versions are failing. It doesn't seem like I've changed anything significant since yesterday, and yesterday's builds were passing the tests.

Also, please add contribution guidelines of some sort, just to clarify how to develop for valtio.

[stephenh]

Feels like we never reach an ideal solution.

Fwiw @dai-shi if the "gah classes" gives you too much grief, I think a potential compromise is requiring users to opt-in via a base class, like:

// in valtio
abstract class BaseStore {
  [hiddenSymbol]: ...
}

// in apps, opt-in to "treat my class as a store"
class User extends BaseStore {}

I know this sort of "forced base class" is a trigger for OO purists, but it would let both the runtime / canProxy do an target instanceof BaseStore and mapped type / Snapshot<T> do an target extends BaseStore and know 100% for sure the user wants their data to be proxified/snapshotted/etc.

I.e. I believe it'd let us remove the "disallow list" of WeakMap+WeakSet+other special things we think of, and move to just "is it a basestore (or the vanilla object literal)"?

I dunno, personally I'd like to avoid that, and try to keep just "vanilla classes" working out-of-the-box. But just mentioning it as a potential middle ground for someday.

[octet-stream]

Fixed Snapshot type tests for TS 3.7. It appears that import type was introduced in 3.8.

[dai-shi]

It appears that import type was introduced in 3.8.

Nice catch! In jotai, we have this patch: https://github.com/pmndrs/jotai/blob/11add6285012fed1c9b25928e3d7ade694e3dd00/.github/workflows/test-old-typescript.yml#L53 but we didn't have it here.. (since jotai's minimal ts version is now 3.8, we can actually delete that line?? 🤔 )

[octet-stream]

Are you hesitant renaming the new file to tests/typings/snapshot.test.ts?

No, but this will require configuration for jest, I think. It should be ignored by it.

[octet-stream]

Didn't catch this one. For some reason VSCodium decided not to show ESLint errors for me :D

[dai-shi]

No, but this will require configuration for jest, I think. It should be ignored by it.

Oh, I wasn't aware that. Which part is bad? We have tests/snapshot.test.ts, so the typings subdir could be something, but not that one...

[octet-stream]

Which part is bad?

The part where jest will likely fail trying to run through this file :) These test files should not be executed in runtime. They only meant for tsc typechecks. But technically we could just ignore the whole typings directory for jest, I guess. Doesn't really matter where these tests placed, or how they named. They're just regular TypeScript files.

I can add configuration for jest and rename the file as you ask. If you insist :)

[dai-shi]

The part where jest will likely fail trying to run through this file :)

Ah, I thought the other way around. Well, we run those type only tests with jest, so, it should be fine. Does it cause CI errors?

[octet-stream]

Well, we run those type only tests with jest, so, it should be fine. Does it cause CI errors?

If jest will ignore test file without any test in it, then it should be fine. I'm not familiar with jest that much, but, for instance, AVA will fail in that case. So I just assumed it might have same behaviour, because it seem reasonable.

[octet-stream]

Google says it will fail. So in that case we need to either ignore these files (as I suggested - the whole tests/typings directory), or add a noop test, or add todo test.

[dai-shi]

This is how it's done in jotai. https://github.com/pmndrs/jotai/blob/main/tests/vanilla/types.test.tsx So, it's a noop test?

[octet-stream]

It does seem like it. I don't see any use of jest's expect function. But they look like they're regular jest tests. I can rewrite these tests like this, if you want.

[octet-stream]

Not sure though why do use have Component at the end of each test.

[dai-shi]

Not sure though why do use have Component at the end of each test.

I think some of eslint rules complain otherwise.

[dai-shi]

I can rewrite these tests like this, if you want.

Yeah, please try it for now. We'll revisit later with other repos. Or maybe with #646.

I may consider including typing tests in tests/snapshot.test.ts for now, as it works with ts 3.7.5. That makes all test files flat.

[octet-stream]

I may consider including typing tests in tests/snapshot.test.ts for now, as it works with ts 3.7.5. That makes all test files flat.

I've merged typings tests file into tests/snapshot.test.ts.