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

Update communicate-with-javascript.md #135

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

davesnx
Copy link
Member

@davesnx davesnx commented Oct 26, 2023

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

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 -->
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
##### Melange attributes
##### Melange-specific attributes

Copy link
Member Author

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? -->
Copy link
Member

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 -->
Copy link
Member

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?
Copy link
Member

Choose a reason for hiding this comment

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

👌

Comment on lines +1081 to +1086
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,
};
Copy link
Member

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 -->
Copy link
Member

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
Copy link
Member

@jchavarri jchavarri Oct 26, 2023

Choose a reason for hiding this comment

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

Suggested change
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” *)
Copy link
Member

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:

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.

@jchavarri
Copy link
Member

jchavarri commented Oct 26, 2023

@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).

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

2 participants