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 bidnings to TextEncoder/TextDecoder #78

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

Conversation

cometkim
Copy link

Both are available on browsers, Node.js, and Deno.

In most cases, it is used without arguments

// UTF-8 encoding/decoding

let e = TextEncoder.make()

e->TextEncoder.encoding // "utf-8"
let bin = e->TextEncoder.encode("Hello, World!")

let d = TextDecoder.make()
d->TextDecoder.decode(bin) // "Hello, World!"

I added some additional bindings to support the full specification.

@TheSpyder
Copy link
Owner

Looks like a good start! Since this is creating a new type rather than reusing something from ReScript, feel free to use the records-as-objects capability of ReScript instead of @get methods:

type t = pri {
  encoding: string,
  fatal: bool,
  ignoreBOM: bool
}

Then your code can look like this:

let e = TextEncoder.make()
e.encoding // "utf-8"

@cometkim
Copy link
Author

cometkim commented Jan 1, 2022

Thanks, @TheSpyder

TIL a binding pattern!

@TheSpyder
Copy link
Owner

Haha yeah it's not obvious but a really nice ergonomic way to do bindings.

So, two more things before we can merge:

  • Add something to the test folder. These files aren't tests we run, they just validate that the correct JS is generated (the JS is checked in to help with this).
  • Changelog entry

@cometkim
Copy link
Author

cometkim commented Jan 8, 2022

There is also TextDecoderStream API in the web, which depends on ReadableStream and WritableStream interface.

Since the WritableStream binding is still empty, I leave it to do.

Copy link
Owner

@TheSpyder TheSpyder left a comment

Choose a reason for hiding this comment

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

Apologies for not picking up on the other thing that needs changing when I reviewed last week.

Comment on lines +11 to +25
%%private(
@new external _makeWithOptions: (string, decoderOptions) => t = "TextDecoder"

@obj
external makeDecoderOptions: (
~fatal: option<bool>=?,
~ignoreBOM: option<bool>=?,
unit,
) => decoderOptions = ""
)

@new external make: unit => t = "TextDecoder"

let makeWithOptions = (~encoding="utf-8", ~fatal=?, ~ignoreBOM=?, ()) =>
_makeWithOptions(encoding, makeDecoderOptions(~fatal, ~ignoreBOM, ()))
Copy link
Owner

Choose a reason for hiding this comment

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

While I appreciate the sentiment to make the bindings more usable, we want these bindings to be zero-cost as much as possible. In this case ReScript 10 should have a new feature that makes makeDecoderOptions unnecessary, but it will be easier to switch to the upcoming style if the exported API takes a decoderOptions.

Please remove the private wrappers and export the more difficult to use way for now; here and below.

Suggested change
%%private(
@new external _makeWithOptions: (string, decoderOptions) => t = "TextDecoder"
@obj
external makeDecoderOptions: (
~fatal: option<bool>=?,
~ignoreBOM: option<bool>=?,
unit,
) => decoderOptions = ""
)
@new external make: unit => t = "TextDecoder"
let makeWithOptions = (~encoding="utf-8", ~fatal=?, ~ignoreBOM=?, ()) =>
_makeWithOptions(encoding, makeDecoderOptions(~fatal, ~ignoreBOM, ()))
@obj
external makeDecoderOptions: (
~fatal: option<bool>=?,
~ignoreBOM: option<bool>=?,
unit,
) => decoderOptions = ""
@new external make: unit => t = "TextDecoder"
@new external makeWithOptions: (string, decoderOptions) => t = "TextDecoder"

Copy link
Collaborator

Choose a reason for hiding this comment

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

You could still have a name argument for the encoding helps people to know that that parameter is supposed to be:
@new external makeWithOptions: (~encoding: string, decoderOptions) => t = "TextDecoder"

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.

None yet

3 participants