Skip to content

runtime(js): better caml_xmlhttprequest_create error handling and remove Map polyfill #1846

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Mar 2, 2025
Merged

runtime(js): better caml_xmlhttprequest_create error handling and remove Map polyfill #1846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b6f5674
runtime(js): remove polyfill for Map to simplify MlObjectTable impl
smorimoto Mar 1, 2025
3c5ad3e
runtime(js): refactor caml_xmlhttprequest_create
smorimoto Mar 1, 2025
74ca496
Update CHANGES.md and ECMASCRIPT.md
smorimoto Mar 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* Compiler: use a Wasm text files preprocessor (#1822)
* Compiler: support for OCaml 4.14.3+trunk (#1844)
* Runtime: support more Unix functions (#1829)
* Runtime: remove polyfill for Map to simplify MlObjectTable implementation (#1846)
* Runtime: refactor caml_xmlhttprequest_create implementation (#1846)

# 6.0.1 (2025-02-07) - Lille

Expand Down
11 changes: 11 additions & 0 deletions ECMASCRIPT.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ Features are grouped by ECMAScript version.
- [Compatibility](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray#browser_compatibility)
- To implement bigarray

### Map

- [Compatibility](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map#browser_compatibility)

## ECMAScript 2016

### async function
Expand All @@ -47,6 +51,7 @@ Features are grouped by ECMAScript version.
- Polyfilled in the repository

### BigInt

- [Compatibility](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt#browser_compatibility)
- For Wasm_of_ocaml

Expand All @@ -58,6 +63,12 @@ Features are grouped by ECMAScript version.
- To implement weak and ephemeron
- Optional

## Web APIs

### XMLHttpRequest

- [Compatibility](https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest#browser_compatibility)

## Node.js

### Built-in modules with `node:` prefix
Expand Down
12 changes: 7 additions & 5 deletions runtime/js/jslib_js_of_ocaml.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,14 @@ function caml_js_get_console() {
//Requires: caml_failwith
//Weakdef
function caml_xmlhttprequest_create(unit) {
if (typeof globalThis.XMLHttpRequest !== "undefined") {
try {
return new globalThis.XMLHttpRequest();
} catch (e) {}
if (typeof XMLHttpRequest === "undefined") {
caml_failwith("XMLHttpRequest is not available");
}
try {
return new XMLHttpRequest();
} catch {
Copy link
Preview

Copilot AI Mar 1, 2025

Choose a reason for hiding this comment

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

Using a catch block without an error binding may obscure useful error details, which could hinder debugging. Consider capturing the error (e.g., using 'catch (e)') to log or examine further.

Suggested change
} catch {
} catch (e) {
console.error("Error creating XMLHttpRequest:", e);

Copilot uses AI. Check for mistakes.

Copy link
Member Author

Choose a reason for hiding this comment

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

This can be added if you prefer @hhugo

Copy link
Member

Choose a reason for hiding this comment

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

I think I prefer the version before this PR. XMLHttpRequest might not be defined (e.g. node) and the code reflected that.

Copy link
Member Author

Choose a reason for hiding this comment

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

So instead of an error message that it could not be created, should it be an error message saying it is not available?

Copy link
Member Author

@smorimoto smorimoto Mar 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Or

function caml_xmlhttprequest_create(unit) {
  if (typeof globalThis.XMLHttpRequest === "undefined") {
    caml_failwith("XMLHttpRequest is not available");
  }

  try {
    return new globalThis.XMLHttpRequest();
  } catch {
    caml_failwith("Failed to create XMLHttpRequest");
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

Maybe but this non longer fit in the PR title and description as it's no longer a simplification and no longer remove checks

Copy link
Member Author

Choose a reason for hiding this comment

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

@hhugo Well, I just updated the title and description of the PR, but if it's better to drop the commit itself, I will drop it.

caml_failwith("Failed to create XMLHttpRequest");
}
caml_failwith("Cannot create a XMLHttpRequest");
}

//Provides: caml_js_error_of_exception
Expand Down
29 changes: 3 additions & 26 deletions runtime/js/marshal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,32 +630,9 @@ function caml_marshal_data_size(s, ofs) {
}

//Provides: MlObjectTable
var MlObjectTable;
if (typeof globalThis.Map === "undefined") {
MlObjectTable = (function () {
/* polyfill (using linear search) */
function NaiveLookup(objs) {
this.objs = objs;
}
NaiveLookup.prototype.get = function (v) {
for (var i = 0; i < this.objs.length; i++) {
if (this.objs[i] === v) return i;
}
};
NaiveLookup.prototype.set = function () {
// Do nothing here. [MlObjectTable.store] will push to [this.objs] directly.
};

return function MlObjectTable() {
this.objs = [];
this.lookup = new NaiveLookup(this.objs);
};
})();
} else {
MlObjectTable = function MlObjectTable() {
this.objs = [];
this.lookup = new globalThis.Map();
};
function MlObjectTable() {
this.objs = [];
this.lookup = new globalThis.Map();
}

MlObjectTable.prototype.store = function (v) {
Expand Down
Loading