Skip to content
This repository was archived by the owner on Dec 17, 2025. It is now read-only.

Fixed DataView. #25

Open
LoganGrier wants to merge 15 commits into
bloodyowl:mainfrom
LoganGrier:FIX/dataview
Open

Fixed DataView. #25
LoganGrier wants to merge 15 commits into
bloodyowl:mainfrom
LoganGrier:FIX/dataview

Conversation

@LoganGrier

Copy link
Copy Markdown

DataView

  • All the accessor functions in DataView were completely broken. This is fixed.
  • For set functions where inputs could be truncated or rounded implicitly, I've created two bindings, a set*Truncated/set*Rounded binding, and a set* binding. The second binding only accepts values that cannot be implicitly modified.
  • If a set function has two bindings, so does its corresponding get function: get*Raw and get*. The second binding returns a constrained value.
  • Added a doc file explaining my design decisions and how to use DataView
  • These changes are backwards incompatible with any code that called get* or set*.

BigInt

  • Added gt, ge, lt, le functions

Testing

  • All functions added/modified have tests
  • Added a github test workflow.

Other

  • package.json scripts are now sorted alphabetically
  • Upgraded to latest version of ReScript.

@bloodyowl

Copy link
Copy Markdown
Owner

hi! thanks for your PR.

I have a few questions & notes, as this PR doesn't fit rescript-js's approach.

All the accessor functions in DataView were completely broken

what do you mean?

  • For set functions where inputs could be truncated or rounded implicitly, I've created two bindings, a setTruncated/setRounded binding, and a set* binding. The second binding only accepts values that cannot be implicitly modified.
  • If a set function has two bindings, so does its corresponding get function: getRaw and get. The second binding returns a constrained value.

I'm not willing to switch to the ConstrainedTypes API here, this library is general-purpose and aims to be zero-cost.

For the littleEndian param, I think I'd be a better API to expose getXWithLittleEndian to avoid having currying issues.

@LoganGrier

LoganGrier commented Sep 14, 2022

Copy link
Copy Markdown
Author

Hi! Thanks for your feedback.

what do you mean?

Accessor functions take a non-optional byteOffset parameter.

I'm not willing to switch to the ConstrainedTypes API here, this library is general-purpose and aims to be zero-cost.

We have different definitions of zero-cost. To me, zero-cost means that abstractions are zero-cost. If a client wants to use an abstraction, it shouldn't cost more that hand-writing the JavaScript that implements that abstraction.

In the PR, if a client don't want ConstrainedTypes, they can call the truncate/rounded/raw function and avoid the overhead. If they use a bundler like webpack, ConstrainedTypes wouldn't even be included in their bundle. If they want to avoid implicit conversions, and verify that they aren't doing implicit conversions, then ConstrainedTypes does this efficiently.

If I understand your definition of zero-cost correctly, it's about not offering abstractions that don't exist in the JS standard library if using those abstractions has a cost. I disagree with this as a design objective. If a client wants the abstraction but doesn't have the option of using it, they either have to go without, and accept the possibility of introducing defects, or write it themselves, which costs engineering time. There aren't any benefits to counterbalance these costs.

ConstrainedTypes offers significant value in my use-case. If I haven't persuaded you to change your mind here, it makes more sense for me to create a separate library that uses ConstrainedTypes than to work on this PR. I don't say this with hostility. This is about creating a binding that meets my needs.

For the littleEndian param, I think I'd be a better API to expose getXWithLittleEndian to avoid having currying issues.

Good point. Agreed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants