Skip to content

Conversation

CedricGuillemet
Copy link
Contributor

No description provided.

@ryantrem
Copy link
Member

So the idea here is to write unit tests in TypeScript with es module imports, bundle it, and execute the bundled unit test code in JsRuntimeHost?

"@types/chai": "^5.2.2",
"@types/mocha": "^10.0.10",
"chalk": "^5.4.1",
"esbuild": "^0.25.6",
Copy link
Member

Choose a reason for hiding this comment

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

I don't know much about esbuild, but have heard some good things and their perf claims sound amazing. That said, do we want a third bundler across our source? Should we just use webpack here? @sebavan

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can use webpack here instead. I have no opinion on which bundler to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now using webpack but getting difficulties getting the .js to run with Chakra. Throwing more babel plugins into the rules doesn't fix it.

@CedricGuillemet
Copy link
Contributor Author

So the idea here is to write unit tests in TypeScript with es module imports, bundle it, and execute the bundled unit test code in JsRuntimeHost?

yes, exactly that.

@CedricGuillemet CedricGuillemet marked this pull request as draft August 1, 2025 08:38
"mocha": "^9.2.2"
"chai": "5.2.0",
"jsc-android": "^241213.1.0",
"mocha": "11.1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using this specific version as more recent ones (at least the one I've tested) errors:

ERROR in node:path
Module build failed: UnhandledSchemeError: Reading from "node:path" is not handled by plugins (Unhandled scheme).
Webpack supports "data:" and "file:" URIs by default.
You may need an additional plugin to handle "node:" URIs.

@CedricGuillemet CedricGuillemet marked this pull request as ready for review August 8, 2025 13:19
Comment on lines +1 to +2
import MochaDefault from "mocha";
const Mocha = MochaDefault as typeof import("mocha");
Copy link
Member

Choose a reason for hiding this comment

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

Is this equivalent to:

Suggested change
import MochaDefault from "mocha";
const Mocha = MochaDefault as typeof import("mocha");
import * as MochaDefault from "mocha";

@@ -60,7 +68,7 @@ describe("AbortController", function () {
});

describe("XMLHTTPRequest", function () {
function createRequest(method, url, body, responseType) {
function createRequest(method: string, url: string, body: any = undefined, responseType: any = undefined): Promise<XMLHttpRequest> {
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 use unknown instead of any?

Suggested change
function createRequest(method: string, url: string, body: any = undefined, responseType: any = undefined): Promise<XMLHttpRequest> {
function createRequest(method: string, url: string, body: uknown = undefined, responseType: unknown = undefined): Promise<XMLHttpRequest> {

@@ -199,7 +207,7 @@ describe("setTimeout", function () {
});

it("should return an id greater than zero when given an undefined function", function () {
const id = setTimeout(undefined, 0);
const id = setTimeout(undefined as any, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe pass in an empty lambda?

@@ -567,7 +583,7 @@ describe("URL", function () {
it("should update href after URLSearchParams are changed", function () {
// Augment URL with pathname and search
const url = new URL("https://httpbin.org/en-US/docs?foo=1&bar=2");
url.searchParams.set("foo", 999);
url.searchParams.set("foo", 999 as any);
Copy link
Member

Choose a reason for hiding this comment

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

Why this cast? Should it just be

Suggested change
url.searchParams.set("foo", 999 as any);
url.searchParams.set("foo", "999");

@@ -617,12 +633,13 @@ describe("URLSearchParams", function () {
const paramsSet = new URLSearchParams("");

it("should throw exception when trying to set with less than 2 parameters", function () {
// @ts-expect-error
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment on what the expected error is and why we need this?

@@ -0,0 +1,13 @@
module.exports = function() {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe some comments about why this is needed.

Copy link
Member

Choose a reason for hiding this comment

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

How does this file relate to the .babelrc at the root?

@@ -2,8 +2,35 @@
"name": "jsruntimehost",
"version": "1.0.0",
"private": true,
"scripts": {
"build": "webpack",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a "watch": "webpack -watch", to make it easier for local dev?

!/node_modules[\\/](?:@babylonjs|mocha|chai)/.test(modulePath)
);
},
use: 'babel-loader',
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 you can put Babel configuration inline here, rather than separate babel specific files. Would this make more sense?

If you prefer to keep it in a separate file, maybe babel.config.js (instead of json or rc) since it is more flexible?

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.

2 participants