Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add runtime type-checker for bigint #366

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sisp
Copy link
Contributor

@sisp sisp commented Jan 21, 2022

No description provided.

@netlify
Copy link

netlify bot commented Jan 21, 2022

✔️ Deploy Preview for mobx-keystone ready!

🔨 Explore the source changes: c67c4e6

🔍 Inspect the deploy log: https://app.netlify.com/sites/mobx-keystone/deploys/6215521a7162960008b67fbf

😎 Browse the preview: https://deploy-preview-366--mobx-keystone.netlify.app

@codecov
Copy link

codecov bot commented Jan 21, 2022

Codecov Report

Merging #366 (c45fc71) into master (0bc6a0d) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #366      +/-   ##
==========================================
- Coverage   90.93%   90.91%   -0.02%     
==========================================
  Files         179      179              
  Lines        6387     6395       +8     
  Branches     1157     1160       +3     
==========================================
+ Hits         5808     5814       +6     
- Misses        542      544       +2     
  Partials       37       37              
Impacted Files Coverage Δ
src/types/primitiveBased/primitives.ts 94.73% <0.00%> (-3.27%) ⬇️
src/types/types.ts 86.36% <0.00%> (+0.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0bc6a0d...c45fc71. Read the comment docs.

@xaviergonz
Copy link
Owner

xaviergonz commented Jan 23, 2022

Hmm, the problem with that types.bigint implementation is that it is not serializable to json, so it would result on unseralizable snapshots/patches when JSON.parse/JSON.stringify is used.

The other option would be to define const types.bigint = typesRefinement(typesString, (n) => /* here check string is a non empty integer number */, "bigint").withTransform(stringToBigIntTransform) or similar, but that would be kind of new (there hasn't been a type with a built-in transform before), but I guess this would be as a good time as any to add it.

@xaviergonz xaviergonz self-requested a review January 23, 2022 15:18
@sisp
Copy link
Contributor Author

sisp commented Jan 23, 2022

Yes, you're right. Your observation is related to #369, something I stumbled over while working on those BigInt features.

@xaviergonz
Copy link
Owner

xaviergonz commented Jan 23, 2022

That being said, if there's a types.bigint with a built-in transform probably there should be a types.date("timestamp" | "isoString") as well... (I'm not implying that should be part of this PR, just thinking out loud :) )

@sisp
Copy link
Contributor Author

sisp commented Jan 23, 2022

Something like types.<someType>.withTransform(...) is along the lines of what I was vaguely trying to say in #369 (comment). Runtime types might ultimately not only check types but form a data schema including data encoding/decoding / serialization/deserialization + while at it also default values for optional (nested) fields.

@sisp
Copy link
Contributor Author

sisp commented Jan 23, 2022

That being said, if there's a types.bigint with a built-in transform probably there should be a types.date("timestamp" | "isoString") as well... (I'm not implying that should be part of this PR, just thinking out loud :) )

As an additional benefit, types with built-in transforms would be awesome when used in objects/arrays and nested fields:

tProp(types.object(() => ({
  int: types.bigint,
  date: types.date("timestamp"),
})))

Currently, it would be hard to do that with the tProp(...).withTransform(...) API.

@xaviergonz
Copy link
Owner

I tried it, even had a whole branch dedicated to that idea, but in the end I got bitten by some of the same typing problems than MST (problems with recursive types mostly)...

@sisp
Copy link
Contributor Author

sisp commented Jan 23, 2022

Hm, do you still have that branch and would you share it?

@xaviergonz
Copy link
Owner

xaviergonz commented Jan 24, 2022

I will need to check if it's still alive (it was on the laptop that broke and not pushed anywhere)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants