-
Notifications
You must be signed in to change notification settings - Fork 6
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
Update communicate-with-javascript.md #135
base: master
Are you sure you want to change the base?
Conversation
I managed to fix some of the feedback I found while revisiting the documentation with JS. I tried to suggest stuff and annotate some of my feedback as comments. We can discard as many as we want, no need to apply any of that, but would be nice to have some fixes
@@ -30,6 +32,8 @@ javascript add : int -> int -> int = {|function(x,y){ | |||
But this would break compatibility with OCaml, which is one of the main goals of | |||
Melange. | |||
|
|||
I would explain this positively instead. Instead of what melange needs to add, can we explain what’s the actual mechanism? Like OCaml with C bindings: https://v2.ocaml.org/manual/intfc.html#:~:text=User primitives are,C-function-name --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this is a bit confusing. I would probably remove all of it and just write a small intro saying something like
Attributes and extension nodes allow to annotate OCaml code to help with code generation, or express functionality that is not supported by default in the language. They look like this:
let add = [%mel.raw "a + b"]
let [@mel.as "POST"] js_post = post
We will see both kinds in the next subsections.
wdyt?
@@ -117,7 +121,7 @@ var student_name = "alice"; | |||
Other OCaml pre-built attributes like `alert` or `deprecated` can be used with | |||
Melange as well. | |||
|
|||
##### Defining new attributes | |||
##### Melange attributes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
##### Melange attributes | |
##### Melange-specific attributes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that sounds good
@@ -919,6 +923,8 @@ function f (x,y) { | |||
Melange provides a relatively type safe approach to use globals that might be | |||
defined either in the JavaScript runtime environment: `mel.external`. | |||
|
|||
<!-- lol, I didn’t know about mel.external. Rather rare considering `external` is a keyword. What about mel.global? --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you feel strongly about this, it should be an issue in https://github.com/melange-re/melange. There's not much to do from the docs perspective until the renaming is implemented.
@@ -1034,6 +1040,8 @@ JavaScript objects are used in a variety of use cases: | |||
- As a class. | |||
- As a module to import/export. | |||
|
|||
<!-- Mention that Everything in JavaScript is an object --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate? Some things in JS are not an object: strings, numbers, booleans,... are primitives:
let str = "neal";
str.age = 22;
console.log(str.age);
@@ -1146,12 +1163,14 @@ var value = [ | |||
]; | |||
``` | |||
|
|||
#### Using `Js.t` objects | |||
#### “Objects with extensible shape” or unknown shape? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
I would explain how to create a record "manually" and latter as a bind | ||
let reason_person = { | ||
name: "Javier", | ||
friends: [| "David", "Antonio", "Jordi" |], | ||
age: 99, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean, you would swap the order of the sections "Using OCaml records" and "Objects with extensible shape or unknown shape"?
|
||
Alternatively to records, Melange offers another type that can be used to | ||
produce JavaScript objects. This type is `'a Js.t`, where `'a` is an [OCaml | ||
object](https://v2.ocaml.org/manual/objectexamples.html). | ||
|
||
<!-- I would explain the difference here, since it's a source of confusion for some users --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two paragraphs just below that mention the differences:
- The first one mentions "no type declaration is needed in advance"
- The second one mentions "an object type can be coerced to another with fewer values or methods"
What other differences do you think we should mention, or in what order?
@@ -1180,7 +1199,7 @@ var john = { | |||
var t = john.name; | |||
``` | |||
|
|||
Note that object types allow for some flexibility that the record types do not | |||
Note that Js.t objects types allow for some flexibility that the record types do not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that Js.t objects types allow for some flexibility that the record types do not | |
Note that `Js.t` object types allow for some flexibility that the record types do not |
@@ -1442,6 +1461,7 @@ like with objects. But unlike objects, there is no need to add any attributes: | |||
|
|||
```ocaml | |||
(* Abstract type for `timeoutId` *) | |||
(* Explain that abstract type here is a super nicety of abstract types. “We could bind to integer, that would be correct, but by being abstract we ensure more safety and express intentionaly” *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a whole section for abstract types at the top of the document:
### Abstract types |
Do you think this addition should be added over there? To have all the info about abstract types upfront, and avoid having to include it in the examples to explain it.
@davesnx thanks for the suggestions. I think I asked or addressed all of them. Once we go through them and get to an understanding / agreement, I can open a PR that just applies them (or just use this one). |
I managed to fix some of the feedback I found while revisiting the documentation with JS.
I tried to suggest stuff and annotate some of my feedback as comments.
We can discard as many as we want, no need to apply any of that, but would be nice to have some fixes