-
Notifications
You must be signed in to change notification settings - Fork 2
JavaScript: have File.setContents() take only the contents as a parameter
#536
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
Conversation
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.
Pull request overview
This PR updates the JavaScript bindings so File.setContents() accepts the file contents directly (e.g., a Uint8Array) rather than a (pointer, length) pair, aligning the JS API with the C++/Python ergonomics and simplifying test/demo code.
Changes:
- Update the JS binding of
File.setContents()to take a single JS value and convert it into bytes. - Refactor JavaScript binding tests and the browser demo to pass
Uint8Arraycontents directly (removing explicit_malloc/_freeusage in most places). - Update version-bumping scripts and bump
VERSION.txt.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/bindings/javascript/solver.secondorderrungekutta.test.js | Switch to 1-arg setContents() and drop manual wasm memory management. |
| tests/bindings/javascript/solver.kinsol.test.js | Same API update across KINSOL tests. |
| tests/bindings/javascript/solver.heun.test.js | Same API update for Heun solver tests. |
| tests/bindings/javascript/solver.fourthorderrungekutta.test.js | Same API update for RK4 tests. |
| tests/bindings/javascript/solver.forwardeuler.test.js | Same API update for Forward Euler tests. |
| tests/bindings/javascript/solver.cvode.test.js | Same API update across CVODE tests. |
| tests/bindings/javascript/solver.coverage.test.js | Coverage tests updated to 1-arg setContents(). |
| tests/bindings/javascript/sed.serialise.test.js | Serialisation tests updated to 1-arg setContents(). |
| tests/bindings/javascript/sed.instance.test.js | Instance tests updated, but one call site still uses the old 2-arg form. |
| tests/bindings/javascript/sed.coverage.test.js | SED coverage tests updated to 1-arg setContents(). |
| tests/bindings/javascript/sed.basic.test.js | Basic SED tests updated to 1-arg setContents(). |
| tests/bindings/javascript/res/res/libopencor.js | Demo now uses FileReader result and passes a Uint8Array to setContents(). |
| tests/bindings/javascript/res/index.html | Minor UI text spacing fix. |
| tests/bindings/javascript/logger.coverage.test.js | Logger coverage tests updated to 1-arg setContents(). |
| tests/bindings/javascript/file.type.test.js | File type tests updated to 1-arg setContents(). |
| tests/bindings/javascript/file.coverage.test.js | Mostly updated, but still contains an old 2-arg setContents() call. |
| tests/bindings/javascript/file.child.test.js | Helper refactored to pass contents directly instead of ptr/len. |
| tests/bindings/javascript/file.basic.test.js | Basic file tests updated to 1-arg setContents(). |
| src/bindings/javascript/file.cpp | Changes the embind signature to accept a single JS value and convert to bytes. |
| newversion.bat | Reworked version bumping to derive from VERSION.txt + date (needs quoting fixes). |
| newversion | Same as above for *nix script (needs quoting fixes). |
| VERSION.txt | Version bumped from 0.20260210.0 to 0.20260211.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… length, as a parameter.
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.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes #535.