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

Convert exports to closures export system and add a ES6 module build. #1235

Merged
merged 1 commit into from
Feb 2, 2025

Conversation

basicer
Copy link
Contributor

@basicer basicer commented Jan 31, 2025

This fixes compatibility issues when used with vite inside a service worker, and should be a bit more idiomatic in general.

src/cpu.js Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@copy
Copy link
Owner

copy commented Feb 2, 2025

Thanks! This looks useful to some people.

I haven't used modules yet, is there a path (not necessary in this PR) to removing libv86.js and only shipping this version? How do other JS projects handle this?

@copy copy merged commit a541757 into copy:master Feb 2, 2025
3 checks passed
@basicer
Copy link
Contributor Author

basicer commented Feb 2, 2025

Thanks! This looks useful to some people.

I haven't used modules yet, is there a path (not necessary in this PR) to removing libv86.js and only shipping this version? How do other JS projects handle this?

I was thinking a bit about this the other day. A lot of libraries these days just have each individual file be a module, they import from each other, and they don't use anything like closure. The upstream project will use something like rollup or webpack to pack it into a more compatible non-esm format as well as minimization and tree-shaking. The biggest problem with this though, as far as v86, is handling DEBUG. There's not really a good way to expose that idea as pure collection of modules. Also I started going though trying to setup everything to be a module and it was a giant pain, there were at least a few circular dependencies between files, and it didnt seem to be worth it.

Sticking with closure, the only real benefit of keeping the non mjs version around is that you can use it in a browser in non-module mode without having to use something like rollup to build a more compatible version of their site. It wouldn't be that hard to convert the site to use the mjs version, but we'd loose a bit of browser compat.

@copy
Copy link
Owner

copy commented Feb 2, 2025

Also I started going though trying to setup everything to be a module and it was a giant pain, there were at least a few circular dependencies between files, and it didnt seem to be worth it

I tend to agree, although it would be nice to remove the requirement to use closure compiler for libv86.js users.

Sticking with closure, the only real benefit of keeping the non mjs version around is that you can use it in a browser in non-module mode without having to use something like rollup to build a more compatible version of their site. It wouldn't be that hard to convert the site to use the mjs version, but we'd loose a bit of browser compat.

That sounds fine (for me, closure would probably remove the exports when creating a single file). But what about node users that are still using require?

Also, I just noticed that the following pattern seems to be imcompatible with imports:

if(typeof performance === "object" && performance.now)
{
    v86.microtick = performance.now.bind(performance);
}
else if(typeof require === "function")
{
    const { performance } = require("perf_hooks");
    v86.microtick = performance.now.bind(performance);
}

When using the mjs, require is not defined, but the import statement has to be moved to the top level.

@basicer
Copy link
Contributor Author

basicer commented Feb 3, 2025

Yeah, its a bit tricky to deal with require. In esm you would use import expression, but unlike require it returns a promise. performance seems to be top level in node now, so its a bit of a moot point for those versions.

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