NPM and Typescript support

[ambientlight]

• package.json with base version set to 0.0.1
• typescript definitions for V86Starter exposed APIs

I have used v86 as a package name

[giulioz]

@ambientlight @copy instead of publishing more mirrors of v86 on NPM, why don't we all maintain the same one? Otherwise it's easy for people to get unmaintained versions and be frustrated with the library. I can share the write access if anybody wants.

Regarding this PR: very nice work with the TS typings! I think, though, that when publishing it on NPM it should be a module, so that users can import it using bundlers. That's the reason of existence of my fork, but I don't know if @copy wants to have his original work as module also.

[ambientlight]

@ambientlight @copy instead of publishing more mirrors of v86 on NPM, why don't we all maintain the same one? Otherwise it's easy for people to get unmaintained versions and be frustrated with the library. I can share the write access if anybody wants.

That would be the best, certainly. I am fine with any way @giulioz and @copy think it is best, this draft is what I added few month back for my own experiment.

Regarding this PR: very nice work with the TS typings! I think, though, that when publishing it on NPM it should be a module, so that users can import it using bundlers. That's the reason of existence of my fork, but I don't know if @copy wants to have his original work as module also.

So here is why this PR is a draft:

I am personally consuming it via importmaps for now:

<script type="importmap">
  {
    "imports": {
      "@glasssh/v86": "/libv86.js",
      "xterm": "./xterm.js",
      "xterm-addon-fit": "./xterm-addon-fit.js"
    }
  }
</script>

I think what we can do is probably make closure compiler produce a UMD bundle (I haven't checked @giulioz's fork yet), the UMD can be then distributed and served with both NPM and CDN style. What do you think? I want to do this in the scope of this PR and then make sure it is smooth with bundling via something like rollup or webpack.

I don't remember how to make typescript work smoothly with global-scoped and module at the same time, but that's my idea for now. I see other libraries just shipped typescript for moduled bundle and then I need to reimport myself the definitions to the global namespace via something like: global.d.ts:

declare namespace globalThis {
    export { Terminal } from 'xterm'
}

[ambientlight]

@copy: I think what was also discussed in a gitter, is that this PR should also add the github action that builds of master and publishes to npm with a revision number (git rev-list --count master). Quick question: what is this revision number used for? Should this action run when version tags are pushed?

[giulioz]

I think what we can do is probably make closure compiler produce a UMD bundle (I haven't checked @giulioz's fork yet), the UMD can be then distributed and served with both NPM and CDN style. What do you think? I want to do this in the scope of this PR and then make sure it is smooth with bundling via something like rollup or webpack.

I've tried a lot with it and unfortunately it's quite difficult to produce module-friendly javascript with closure compiler, so I ended up removing it completely and using Rollup instead. I also have performed some tests and the performance benefits introduced by closure compiler are none, so there should be no harm in doing that.

The main problem is that v86 relies a lot on working with the global scope, which is difficult to handle when using bundler and typescript. My solution in the fork was to explicitly import and export everything so that it can avoid polluting the global namespace.

[copy]

instead of publishing more mirrors of v86 on NPM, why don't we all maintain the same one?

Absolutely agreed. Let's do this now, since we're all here.

That's the reason of existence of my fork, but I don't know if @copy wants to have his original work as module also.

I would love to have your changes in the repository. If we can remove the horrible loader code in debug.html — even better. My only requirement is no mandatory npm dependencies, the release build should go through closure compiler, and no build step for debug.html.

I think what we can do is probably make closure compiler produce a UMD bundle (I haven't checked @giulioz's fork yet), the UMD can be then distributed and served with both NPM and CDN style. What do you think? I want to do this in the scope of this PR and then make sure it is smooth with bundling via something like rollup or webpack.

That would be great, and I'd strongly prefer this over using rollup inside of this repository (see the next paragraph).

I've tried a lot with it and unfortunately it's quite difficult to produce module-friendly javascript with closure compiler, so I ended up removing it completely and using Rollup instead. I also have performed some tests and the performance benefits introduced by closure compiler are none, so there should be no harm in doing that.

The main problem is that v86 relies a lot on working with the global scope, which is difficult to handle when using bundler and typescript. My solution in the fork was to explicitly import and export everything so that it can avoid polluting the global namespace.

Removing Closure Compiler entirely is not really an option for me. It does bundling, minifying, basic linting, type-checking, and is much more stable than most things in the npm ecosystem (and it can be installed through npm for the folks who don't want to install java). We could use Rollup only for the npm build, but having a single build system would obviously be nicer.

I'm not opposed to refactoring the global stuff if it makes bundling easier.

I think what was also discussed in a gitter, is that this PR should also add the github action that builds of master and publishes to npm with a revision number (git rev-list --count master). Quick question: what is this revision number used for? Should this action run when version tags are pushed?

It is used as the version number (perhaps prefixed with 0.0., so if we decide to do versioning, we can bump the prefix). No need to handle version tags for now.

[giulioz]

Removing Closure Compiler entirely is not really an option for me. It does bundling, minifying, basic linting, type-checking, and is much more stable than most things in the npm ecosystem (and it can be installed through npm for the folks who don't want to install java). We could use Rollup only for the npm build, but having a single build system would obviously be nicer.

True, but closure compiler has a really hard time producing "modern" js understandable by bundlers. Would you be ok with having both of them?

I've tried to use closure to make importable modules, but the documentation about that non-existent. Do you have any info for that?

[ambientlight]

@giulioz, @copy: Thanks for great input, understood.

@giulioz: I was thinking more of a scaffold like at https://stackoverflow.com/a/39862418/2380455, however I have limited experience with closure compiler.

I'm seeing a non-minified libv86-debug.js having the following structure:

It should fit in to the UMD scaffold if we substitute those with referenced scaffold in the SO's answer. The UMD should then be consumable with tooling like bundlers that understand commonjs or amd. This is quite hypothetic though as I haven't tried it yet.

[copy]

True, but closure compiler has a really hard time producing "modern" js understandable by bundlers. Would you be ok with having both of them?

I'm not strongly opposed to having both, but it would be nicer if everything was built using a single tool.

I've tried to use closure to make importable modules, but the documentation about that non-existent. Do you have any info for that?

There is some support for exports with the @export annotation. It prevents renaming and generates this code:

V86Starter.prototype.run = function() {
   ...
};
goog.exportProperty(V86Starter.prototype, "run", V86Starter.prototype.run);

(where goog.exportProperty is empty by default, and I believe you could define it yourself).

Indeed there seems to be no support for generating export statements (which I guess would be the most compatible method?)

I see two options:

1. We put everything that should be exported into a (global?) object and append export default … to the output file
2. We modify the existing export code to make it compatible with whatever bundlers expect

[ambientlight]

I see two options: 1. We put everything that should be exported into a (global?) object and append export default … to the output file 2. We modify the existing export code to make it compatible with whatever bundlers expect

@copy, @giulioz : 2 seems to be cleaner (removing those window[...] = ), together with a change to https://github.com/copy/v86/blob/d685053541260fcf4586bce20420d4b0edd32eeb/Makefile#L136 with some kind of a scaffold that collects the definitions seems to do the trick:

--output_wrapper 'var v86=(function(){%output% return {V86Starter,CPU}}).call(this);if(typeof module === 'object' && module.exports){module.exports = v86;}'\

need to play around with this more, maybe there is a cleaner way for this.

But without code changes, I have failed with my original idea of tampering around the compiled bundle via changing that --output-wrapper. Generated code adds some magic that finds the global scope and then attaches the definitions just as it should, or I am missing something. From what I see, existing output wrapper helps for closure compiler to not inject its polyfills and internal flags into global scope. But I don't seem to find a way how to hijack it and have an empty module assumed instead of global. If we don't perform code modification, I am unsure to get rid of having v86 in global scope. If we are ok with global scope however, we can provide guides to the user to make their bundling work in webpack and rollup probably. It needs tweaks but webpack is quite straightforward for example. the webpack 5 configuration I have used to bundle v86 is something like:

const path = require('path');
const isProduction = process.env.NODE_ENV == 'production';
const config = {
    entry: './src/index.wp.ts',
    output: {
        path: path.resolve(__dirname, 'dist'),
    },
    module: {
        rules: [
            {
                test: /\.(js|jsx|tsx|ts)$/i,
                loader: 'babel-loader',
                exclude: ['/node_modules/'],
            }
        ],
    },
    resolve: {
        extensions: ['.tsx', '.ts', '.js'],
        fallback: {
            "crypto": false,
            "perf_hooks": false,
            "fs": false,
            "path": false
        }
    },
    externals: {
        './libwabt.js': 'WabtModule',
        './capstone-x86.min.js': 'MCapstone'
    }
};

module.exports = () => {
    if (isProduction) {
        config.mode = 'production';
    } else {
        config.mode = 'development';
    }
    return config;
};

[kyr0]

I'm testing your typings and I found one missing in V86StarterOptions. At least in the version I use, you can set wasm_fn and I use that option because I import the WASM module this way via Vite / esbuild:

import v86Wasm from "v86/build/v86.wasm";

Now the typings result in:

Argument of type '{ wasm_fn: (options: WebAssembly.Imports) => Promise<WebAssembly.Exports>; memory_size: number; vga_memory_size: number; screen_container: HTMLElement; bios: { ...; }; vga_bios: { ...; }; fda: { ...; }; autostart: true; }' is not assignable to parameter of type 'V86StarterOptions'.
  Object literal may only specify known properties, and 'wasm_fn' does not exist in type 'V86StarterOptions'

To fix this, you can simply add to V86StarterOptions:

    /**
     * Reference to the v86 wasm exorted function.
     * @default undefined 
     */
    wasm_fn: (options: WebAssembly.Imports) => Promise<WebAssembly.Exports>

[kyr0]

Btw, I'm working on the "internal" typings as well. I use them because I, despite reading the mem8, also read the registers of the CPU, reg32, directly (for example).

At the moment, v86 as a property of V86Starter is missing. I'd like to share my typings as well. I have it structured like this (it's a ton of typings, I'm outlining here):

export class CPU {
    ...
}

declare class V86 {
    ...
    cpu: CPU
    ...
}

declare class V86Starter {
   ...
   v86: V86
   ...
}

What would be the best way to join this PR? Would you guys like to merge this first and I open another one on top of this?

btw, another one: If someone is testing this and already using one of the existing NPM packages out there, they will run in the error below until all of the definitions are wrapped in: declare module 'v86' { ... } (or whatever the npm module name they use is).

Of course this wouldn't be a problem for the when this package is released, as long as we include types or typings field pointing to the .d.ts file in package.json.

[ambientlight]

@kyr0: thanks for the feedback, I suspect the typings are incomplete - as at the moment of me writing them - I wasn't much familiar with v86, so anything is certainly very helpful for this PR, thanks.

What would be the best way to join this PR? Would you guys like to merge this first and I open another one on top of this?

Sure, you can just add your typing directly to this PR, I can give your write access to the fork (I have sent you the invite)

btw, another one: If someone is testing this and already using one of the existing NPM packages out there, they will run in the error below until all of the definitions are wrapped in: declare module 'v86' { ... } (or whatever the npm module name they use is).

I will have this addressed now as per original @copy's comment. For now the typing here assume global scope.

@copy: I also want to clarify on what should be exposed in total (I just scanned the codebase for those exports snippets). Is it like?:

import { v86, V86Starter, CPU, MemoryFileStorage, ServerFileStorageWrapper } from "v86"

[copy]

@copy: I also want to clarify on what should be exposed in total (I just scanned the codebase for those exports snippets). Is it like?: import { v86, V86Starter, CPU, MemoryFileStorage, ServerFileStorageWrapper } from "v86"

V86Starter has been renamed to V86, and I don't want to expose the name v86 (I actually want to get rid of it).

[kyr0]

@ambientlight You're welcome. I'll add a few hundred more lines tomorrow. Btw, depending on the configuration (no implicit any) tsc is a bit unhappy about functions without explicit return type like this one here: https://github.com/copy/v86/pull/647/files#diff-093ad82a25aee498b11febf1cdcb6546e4d223ffcb49ed69cc275ac27ce0ccceR515 -> serial_send_bytes(serial: number, data: Uint8Array) (but there are more); I fixed most of them them on my side, but maybe you wanna add them for the ones you might add on top of the existing ones?

[kyr0]

Hey @ambientlight @copy , sry, I'm sick since yesterday. I'll get back to this once I've recovered. I hope for tomorrow..