Skip to content

Add basic support for String.prototype.char_at() #286

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

Closed
wants to merge 1 commit into from

Conversation

elpiel
Copy link
Contributor

@elpiel elpiel commented Jun 21, 2018

I really would love to get some feedback.
Also one question I couldn't answer myself - what about optional parameters?
char_at() has index, which by default should be 0, but in Rust there is no way to do it and I am not quite sure I understand the wasn-bindgen enough for that. Also some other implementations with optional value don't have it yet, e.g. Array.prototype.includes()

I hope I can fully finish this PR.

PS: Currently the Array and Object are not in Alphabetical order as in the Guidelines.

import * as assert from "assert";
import * as wasm from "./out";

var anyString = 'Brave new world';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is taken from the documentation. I haven't added a test case for default index = 0 as I am not sure currently it's possible

@jonathan-s
Copy link
Contributor

Nice :). I was trying to make either, .length or .includes() work for String. But got obscure errors for both. If you happen to solve one of those I'd be interested to see what went wrong. Code for .length can be found over at #281

Also it looks like you're getting some errors on array for types, it's now expecting js::String instead of String. I'd go ahead and change the expected type for the string ones.

@elpiel
Copy link
Contributor Author

elpiel commented Jun 21, 2018

Can you give me more details about that change, I am not sure how to use the appropriate js::String only for the new String

@fitzgen
Copy link
Member

fitzgen commented Jun 21, 2018

Regarding defaults and optional parameters: because we don't currently have good support for these features, we have to make a decision about what to do on a per-case basis. In this case, I don't think the default behavior is useful enough that we want to expose it, and should simply require that folks provide the index.

   Compiling test67 v0.0.1 (file:///home/travis/build/rustwasm/wasm-bindgen/target/generated-tests/test67)
error[E0369]: binary operation `==` cannot be applied to type `wasm_bindgen::js::String`
  --> src/lib.rs:11:17
   |
11 |                 assert_eq!(x, "ABC%20abc%20123");
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: an implementation of `std::cmp::PartialEq` might be missing for `wasm_bindgen::js::String`
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
error[E0277]: `wasm_bindgen::js::String` doesn't implement `std::fmt::Debug`
  --> src/lib.rs:11:17
   |
11 |                 assert_eq!(x, "ABC%20abc%20123");
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `wasm_bindgen::js::String` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug`
   |
   = help: the trait `std::fmt::Debug` is not implemented for `wasm_bindgen::js::String`
   = note: required because of the requirements on the impl of `std::fmt::Debug` for `&wasm_bindgen::js::String`
   = note: required by `std::fmt::Debug::fmt`
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

These errors are occurring because the new extern type String (aka wasm_bindgen::js::String) is over-riding rust's prelude's String, so all the existing APIs that take/return String are now using wasm_bindgen::js::String, and it doesn't implement Debug or PartialEq<str>, both of which are needed by the assert_eq! macro.

Eventually, there is a more fundamental question of whether we should eagerly copy JS strings into the wasm heap as Rust strings or not. Right now, we should continue following the existing behavior. We can do that by calling JS strings JsString:

#[wasm_bindgen]
extern {
    #[wasm_bindgen(js_name = String)]
    pub extern type JsString;
}

If this doesn't work, then I think we should skip adding String bindings until we can fix whatever underlying issue we run into.

@jonathan-s
Copy link
Contributor

jonathan-s commented Jun 21, 2018

@fitzgen It does not seem to work to call it JsString. I just tried that out on this branch. The error you get is the following: ReferenceError: JsString is not defined.

@elpiel
Copy link
Contributor Author

elpiel commented Jun 21, 2018

I agree, I still can't make it work like this.

@fitzgen
Copy link
Member

fitzgen commented Jun 21, 2018

Ok, let's skip bindings for JS String until we have the infrastructure in place. Thanks for exploring this y'all!

@fitzgen fitzgen closed this Jun 21, 2018
@fitzgen
Copy link
Member

fitzgen commented Jun 21, 2018

Filed #287 to track fixing the underlying issues here.

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.

3 participants