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

Redesign #23

Merged
merged 131 commits into from
Feb 1, 2019
Merged

Redesign #23

merged 131 commits into from
Feb 1, 2019

Conversation

athanclark
Copy link
Collaborator

An absolute ton went into this revision. Here is a brief summary:

Testing

There's a much more thorough test suite, first off - the TypedArray mechanism was most important - to prove that all generated values, for the different bit-caste sizes, operate exactly as expected for all operations, was a critical milestone.

Foreign Interface

The previous version relied on Uint8Arrays as the fundamental representation in JavaScript for most types - that is no longer the case; the actual JavaScript DataView, ArrayBuffer, and TypedArray types are the underpinning representation for the PureScript types.

Class-based API

Because of the commonality between APIs among the different TypedArray types, I took the liberty of designing it with classes and instances in-mind. The performance overhead should be negligible, especially when using rollup-plugin-purs.

There's also a classy interface for DataViews, to choose the getter and setters for each binary representation.

No more Data.ArrayBuffer.Show, No more String encodings

As @AlexaDeWit mentioned in #20, string encodings will be done with purescript-arraybuffer-codecs. The Show module used a foreign npm module, inspect I think, and it just felt gnarly, so I got rid of it.

Generators

MonadGen values live in Data.ArrayBuffer.*.Gen, for unit test generation.

Type-Level Kit

There's some extra mappings in Data.ArrayBuffer.ValueMapping, where byte length and application-level type casts are defined for each ArrayViewType. This was necessary to keep DataView and TypedArrays consistent at compile-time.

Value-Level Kit

The TypedArray interface at Data.ArrayBuffer.Typed provides all the tools you'd expect from an iterable structure - folds, maps, traversals, searching, querying, filtering, slicing, etc. are all in the form of Snoyman's mono-traversable.

That's about it! Please let me know what you think, any feedback is helpful.

@athanclark
Copy link
Collaborator Author

Oooh, I just had a tricky thought... should offsets and byte length be represented as UInts? Because they can't be negative...? It feels against-the-grain though :\

@jacereda
Copy link
Collaborator

In the manual bounds-checking they're interpreted as UInts... If there was compiler support so that literals could become UInts automagically it would probably be the way to go, but as it's now I guess it will just be burdensome.

@athanclark
Copy link
Collaborator Author

Yeah I was thinking the same. We gotta get some kind of purescript v0.13 wish list going or something


-- | Returns a new typed array view of the same buffer, beginning at the index and ending at the second.
-- |
-- | **Note**: there is really peculiar behavior with `subArray` - if the first offset argument is omitted, or
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this behaviour for subarray documented somewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish. I can't find it in the ECMA spec, but it's pretty obvious if you try it in psci. I'm not sure how to find a doc on this either

foreign import joinImpl :: forall a. Fn2 (ArrayView a) String String

-- | Prints array to a delimiter-separated string - see [MDN's spec](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/join) for details.
toString' :: forall a. ArrayView a -> String -> String
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

join? toStringSep? toStringSep? toStringIntercalate?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll go with join :) sorry

@jacereda
Copy link
Collaborator

I've pushed more changes to the v10 branch making more functions effectful. I guess some more would need to be converted, particularly I guess at should also be effectful, right?

@athanclark
Copy link
Collaborator Author

Definitely, at is just Ref.read in a way

@jacereda
Copy link
Collaborator

I've unified the signatures for slice/subArray/fill to mirror Array/Node.Buffer APIs in the v10 branch. Also, I can't reproduce the behaviour you specify for subArray and removed the note. I seem to remember fixing one of the subArray tests, maybe that was the problem (or maybe you found a broken node version?).

@athanclark
Copy link
Collaborator Author

:s shoot I don't think so, but I wouldn't doubt it. iirc I was using nvm v8.12.0, but either way I'm glad the mystery is cleared up.

But awesome, I definitely should have thought of that - the API mirroring.

exports.asInt8Array = function(v) {
return new Int8Array(v.buffer, v.byteOffset, v.byteLength);
}
exports.polyFill = function polyFill () {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you export this? Would it be problematic to just ejecute the polyfill directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it would, but I was just trying to be on the safe side - it technically causes an effect, and although I know ffi files only get executed once, I just felt like someone might want finer granularity of control. But honestly I think it would be fine to just execute - I pulled it from MDN.

@jacereda
Copy link
Collaborator

jacereda commented Feb 1, 2019

I've added some unsafe instances (Show/Eq/Ord/Semigroup/Monoid), but I don't think these belong in this repo, I guess they could be somewhere else. See #24

@athanclark
Copy link
Collaborator Author

Beautiful! I think it looks great. The newtype was exactly what I was going to suggest.

@jacereda jacereda merged commit 45f310a into purescript-contrib:master Feb 1, 2019
@jacereda
Copy link
Collaborator

jacereda commented Feb 1, 2019

I've merged to master but will hold the release until I get comments from @AlexaDeWit regarding the new API.

On a different subject, do you think TypedArrays could have a Foldable instance? I've been unable to write it...

@athanclark
Copy link
Collaborator Author

Awesome, that will be great. I think a cereal compatible library is next on my plate after that. But technically, no, because it's not a container over "any type a"; only the type that it's related to - like a Uint or whathaveyou; it's got that extra constraint predicate.

@jacereda
Copy link
Collaborator

jacereda commented Feb 1, 2019

But somehow it's a container for "some type a". Is it possible to specify that a could be any of [UInt,Int,Float32,...]?

@athanclark
Copy link
Collaborator Author

No, because an instance of Foldable only exposes it's container type f for an instance constraint. Because Foldable.foldr adds an additional forall a. in it's member definition - there's no way to link that skolemized a with the f at the top level.

@athanclark
Copy link
Collaborator Author

It's almost the same idea for why a Set can't be a Functor - because we have no way to say that the member elements must also have an Ord constraint.

@jacereda
Copy link
Collaborator

jacereda commented Feb 2, 2019

Awesome, that will be great. I think a cereal compatible library is next on my plate after that. But technically, no, because it's not a container over "any type a"; only the type that it's related to - like a Uint or whathaveyou; it's got that extra constraint predicate.

Not cereal compatible, but some time ago I started https://github.com/jacereda/purescript-binary and I guess I'll update it at some point to work with this.

@athanclark
Copy link
Collaborator Author

Ping - hey sorry to pester. Were you intending on publishing the new changes in master? I know the v9.0.0 version doesn't include the Gen modules and all of that. I'm just trying to publish some other libraries that rely on this one :) thanks for your help!

@jacereda
Copy link
Collaborator

jacereda commented Feb 5, 2019

I was just waiting to see if @AlexaDeWit had any comment regarding the new API. In any case, I'll try to release on Thursday.

@athanclark
Copy link
Collaborator Author

Awesome, thank you!

@AlexaDeWit
Copy link
Contributor

Sorry about that, I didn't mean to ignore things. I've just not been following this super closely, as legacy versions will work for me, and I kind of trust you two to come up with something coherent so that when I get around to making my codec lib it should be fine

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

Successfully merging this pull request may close these issues.

None yet

3 participants