Unable to get TypeScript to compile, on grounds that it can't find type declarations for the worker

[StoneCypher]

Right now, I'm trying to convert from mdaines viz to yours, because an unrelated thing forced my hand in upgrading rollup, and following that, my ability to import viz and my actual project at the same time, since one is es6 and the other homebrew UMD, kind of collapsed. I fought mdaines viz into working something like five years ago and it's been pretty copasetic since, but that truce failed with my recent rollup upgrade and I'm flailing for answers. I'm hoping that your alternative packaging in modern notations smooths things out.

At the time of the fault, I'm trying to compile under tsc directly. It seems like your project is typescript, and as such that ought to work.

> jssm-viz@5.77.1 typescript
> tsc --build tsconfig.json

src/ts/jssm-viz.ts:3:36 - error TS2307: Cannot find module '@aduh95/viz.js/worker' or its corresponding type declarations.

3 import getWorker              from "@aduh95/viz.js/worker";
                                     ~~~~~~~~~~~~~~~~~~~~~~~

take a quick look in the file, and

throw new Error('The bundler you are using does not support package.json#exports.')

(Note: the relevant rollup module, @rollup/plugin-node-resolve, does in fact support #exports)

Er. But I'm not even at bundling yet. I just copy pasted your readme instructions. The failing relevant line is the second line here:

import Viz from '@aduh95/viz.js';
import getWorker from "@aduh95/viz.js/worker";

My toolchain is the simple approach: take some ts, run tsc against it to write js to disk, in a build directory; afterwards, run rollup against the js. So rollup hasn't even gotten started yet.

So I looked in dist, and there's no obvious .d.ts for this module

Looked it up through package.json#exports, and it's a .mjs. This suggests that in order to use it I'm going to have to turn allowJs on and checkJs off. Fortunately, I already had those on and off respectively, because of the peg parser.

But still, typescript insists it can't see the types for the declarations. What that suggests to me is that maybe typescript can't find the #exports, and that's ... kind of a problem. I want it to be that I'm misunderstanding.

I think maybe this might be fixable with a .d.ts though?

I don't really know what's going on here. I've never seen package.json#exports before, and it's worrying to rely on fundamentally new things like that, because oftentimes tools along the way are blind to it

 

Repro

You can see the relevant code here, on line 3

You can see the outcome here in the github action context, or if you prefer, here in source context

[StoneCypher]

That's a very fast fix!

Could I get you to re-publish to NPM please?

(Would you like my auto-publish action?)

[aduh95]

Thank you, unfortunately I have a failing test on CI due to conflicting types in the typescript package (You would think that they of all people would have their types figured out, but go figure) that's blocking the release, and I'm not sure how to workaround it. Maybe I'll need to use an older TS version or something.

[StoneCypher]

Wow, that's gross.

It seems like this is a web API whose type has actually genuinely changed. I think maybe trying to fix this with the version of TypeScript will leave you trapped, because TypeScript isn't wrong here, and you're just setting yourself up with a version of TypeScript that's already out of date.

I wonder if you could change the type to be export typeof AbortSignal, so that the host environment just enforces whatever it has locally

If not, I guess one awful thing you could do is compile two versions of the .d.ts, one for older and one for newer

[aduh95]

The frustrating thing is I'm not even using AbortSignal anywhere in my code.

[StoneCypher]

Can you just delete it from the .d.ts then? Or declare it unknown or something?

If I knew how to build the library, I'd be trying to help, instead of just writing silly comments

[StoneCypher]

or, actually, can you generate once with the newer api that has the extra parameter, then manually edit the .d.ts and mark that parameter optional, maybe?

[aduh95]

I've fixed it with a0213df. v3.7.0 is out :)

[StoneCypher]

you're impressively fast and correctness oriented. i'll try it out later tonight, when i'm not under fire

[StoneCypher]

I think this signature isn't what you intended:

declare let exports: (moduleOverrides?: EMCCModuleOverrides) => Promise<EMCCModuleOverrides> | Worker;

I think you just want that to be Worker

Running your instructions now results in

src/ts/jssm-viz.ts:21:26 - error TS2322: Type 'Worker | Promise<EMCCModuleOverrides>' is not assignable to type 'Worker | Worker'.
  Type 'Promise<EMCCModuleOverrides>' is not assignable to type 'Worker | Worker'.
    Type 'Promise<EMCCModuleOverrides>' is missing the following properties from type 'Worker': onmessage, onmessageerror, postMessage, terminate, and 4 more.

21       viz    = new Viz({ worker });
                            ~~~~~~

Found 1 error.

I think the problem is that the import foo from 'bar' syntax is actually ambiguous: it relies on resolving the module to figure out whether that's an implementation or a container object, and the build system can't tell from here whether it's going to be emscripten's container object (which is EMCCModuleOverrides) or some function

You can fix this by just switching to the object destructuring syntax import { foo } from 'bar', but that's a breaking change

Alternately, you might try just manually narrowing that export to remove EMCCModuleOverrides? I don't actually know if that's legal or not

[StoneCypher]

import Viz       from '@aduh95/viz.js';
import getWorker from "@aduh95/viz.js/worker";

function is_worker(w): w is Worker {
  if ('postMessage' in w) { return true; }
  return false;
}

let worker, viz;

async function setup() {

  worker = await getWorker();

  if (worker === undefined) { throw 'Worker undefined!'; }
  if (!(is_worker(worker))) { throw 'Failed import!'; }

  viz = new Viz({ worker });
  (window as any).viz = viz;

}

setup();

worker throws as undefined 😢

[aduh95]

OK, reopening since the issue is not fixed.

I think this signature isn't what you intended [...] I think you just want that to be Worker

It can actually be one or the other depending if you are writing code for Node.js or not. On Node.js, you can use it to create the Worker for you:

viz.js/README.md

Lines 31 to 36 in a638079

	```js
	import Viz from "@aduh95/viz.js";
	import getWorker from "@aduh95/viz.js/worker";
	const worker = getWorker();
	const viz = new Viz({ worker });

On a web browser (or Deno), you have to create the web worker yourself – it's necessary as there's no reliable way to know where to look for the WASM file. In that context, the return type is Promise<EMCCModuleOverrides>, not Worker.

viz.js/README.md

Lines 109 to 131 in a638079

	```js
	// worker.js
	import initWASM from "@aduh95/viz.js/worker";
	// If you are not using a bundler that supports package.json#exports
	// use "./node_modules/@aduh95/viz.js/dist/render.browser.js" instead.
	// You need to configure your bundler to treat `.wasm` file as file to return a URL.
	import wasmURL from "@aduh95/viz.js/wasm";
	// With Rollup, use the `@rollup/plugin-url` plugin and add `**/*.wasm` to the
	// `include` list in the plugin config.
	// With Webpack, use the `file-loader` plugin: "file-loader!@aduh95/viz.js/wasm"
	// If you are not using a bundler that supports package.json#exports
	// or doesn't have a file-loader plugin to get URL of the asset:
	// const wasmURL =
	// new URL("./node_modules/@aduh95/viz.js/dist/render.wasm", import.meta.url);
	initWASM({
	locateFile() {
	return wasmURL;
	},
	});
	```

Then, since you mentioned Rollup, you can use @surma/rollup-plugin-off-main-thread plugin, here's how you can do it:

import Viz from "@aduh95/viz.js";
import VizWorker from "worker-loader!./worker.js";

let viz
async function dot2svg(dot, options) {
  viz ??= new Viz({ worker: new VizWorker });
  return viz.renderString(dot, options);
}

Unfortunately it looks like there's no easy way to tell TypeScript to generate two .d.ts files, ideally we would have one for each env.

worker throws as undefined 😢

Since this code uses window, I'm going to assume it's written for the browser.
That's the expected behavior, await getWorker() will initiate the WASM file and return the argument you passed to it (so in this instance, undefined). As I said earlier, getWorker is really not a good name in this context, initWASM would be more suited.
I'm sure I could improve the types here to tell TypeScript that the function returns whatever it receives, if you know how to do that and what to contribute a PR that'd be awesome.

[StoneCypher]

Okay but worker throws as undefined, so whatever it is you're returning isn't being returned

It's throwing here:

  if (worker === undefined) { throw 'Worker undefined!'; }

[StoneCypher]

if you know how to do that and what to contribute a PR that'd be awesome.

I'm happy to experiment but at this time I still don't know how to build

[aduh95]

Okay but worker throws as undefined, so whatever it is you're returning isn't being returned

I'm returning whatever you're passing to the function. If you pass no parameters, you get undefined, that's expected. As I said, it won't return a Worker on the browser, that's simply not something I can do.

[StoneCypher]

ok, i'm re-reading what you wrote and i'm realizing i misread it last night

so what i actually should be seeing is this:

incidentally, i support both the browser and node

[aduh95]

incidentally, i support both the browser and node

Supporting both is a tough one, there are three ways to do that:

1. Expose two files, one for Node.js, one for the browser, and have a separate set of instructions for each one;
2. Use to the "sync" version (@aduh95/viz.js/sync) that's using ASM-JS instead of WASM. That would work for both, but that's more bytes to send through the wire, more work on the main thread, and potentially subpar performance;
3. Both, and let the users pick the one that makes sense for them. (That's what this package is doing).

Option 2 is the easiest to setup, both for you and for your consumers, but it's arguably the worst one for the end-users. Choice is yours :)

[StoneCypher]

what do you mean by more bytes through the wire?

would that end up with my embedding graphviz twice? it's several meg

[StoneCypher]

i don't really care about performance if by which you mean speed. it's extremely unlikely in my circumstances that you'll see even a hundred nodes in a given graph. this is for rendering state machines, and it's an auxiliary tool.

exposing two files is not realistic in my circumstances

if the size hit is like 5-10% that's fine, but if it's a doubling i don't think i can do that

[aduh95]

what do you mean by more bytes through the wire?

The brotlified size when using the WASM file is 313kiB, it goes to 400kiB when using ASM_JS (it's 128% the size of the WASM version).

would that end up with my embedding graphviz twice? it's several meg

No unless you do something wrong ;)

[StoneCypher]

okay, an 80k increase is tolerable

 

No unless you do something wrong ;)

sir i will have you know that this is my specialty

[aduh95]

That's closer to 90k than 80k, but as long it works for you all is good.

Closing now, as I think the original issue is solved.

[StoneCypher]

So, I tried to include the sync version.

const vizRenderStringSync = require("@aduh95/viz.js/sync");

console.log(vizRenderStringSync("digraph{1 -> 2 }"));

This gives me the fascinating and new

ReferenceError: __dirname is not defined

render_sync.cjs:10 Uncaught ReferenceError: __dirname is not defined
    at render_sync.cjs:10:313
    at render_sync.cjs:10:1702681
    at jssm-viz.js:207:1

It's here:

{{a=__dirname+"/"}o=()=>{if(!u){t=require("fs");u=require("path")}};n=function A(e,i){var r=Qi(e);if(r)

It looks like it's trying to stub out some node stdlib modules. Assuming you're building using rollup, this may be useful

[aduh95]

hum the sync version is much more Node.js oriented, you'd have to jump a few hoops to make it work on the browser (in case you don't know __dirname is a magic variable in CJS that represents the path to the parent folder of the script; of course, that has no meaning in a browser context).

After closer inspection, most of the Node.js specific code can be removed, the only thing you really need is a Buffer polyfill.

[StoneCypher]

in case you don't know __dirname is a magic variable in CJS that represents the path to the parent folder of the script; of course, that has no meaning in a browser context

yeah, that's why i recommended that polyfill. it supports all node's stuff and patches or shims out all the relevant libs

 

After closer inspection, most of the Node.js specific code can be removed, the only thing you really need is a Buffer polyfill.

okay, great. the above polyfill can do that, but if you removed everything but that, i'd think the feross buffer polyfill would probably be the better choice

i'm a little surprised emscripten doesn't provide one

[StoneCypher]

@aduh95 - So I wasn't clear after reading this ticket on whether removing stuff from the repo so that it can be portable is a goal here

Is this something you expect to do? Can I help? That's sort of a total blocker for me, and I would like to know if I should continue with this fork

[aduh95]

So there's a good chunk of Node.js specific code that can be removed from the sync version in this repo, however you'd still need to polyfill some of Node.js APIs – didn't you say you found a Rollup plugin for that?

To clarify, making the sync version compatible with browsers is not a goal (i.e. I don't intend to ship a Buffer polyfill, that would be something that need to happen down the line), I'm fine with maximising the compatibility when we can though.

[StoneCypher]

however you'd still need to polyfill some of Node.js APIs – didn't you say you found a Rollup plugin for that?

Yeah, rollup actually tells you to install it when you accidentally bundle certain standard library modules (the one that usually reminds me is fs.)

It's FredKSchott/rollup-plugin-polyfill-node. It's not actually part of rollup but it quasi-is. It's first-tier community, I guess.

 

To clarify, making the sync version compatible with browsers is not a goal

I wish you would reconsider this. It seems like it would be very helpful for portability.

 

I'm fine with maximising the compatibility when we can though.

This is kind of the bottom line for me, is "can I get this working in both node and the browser"

It's okay for me if I have to do some juggling to make it work, but I'm not willing to include two versions of the script, because NPM ranks people by binary size

I had this working in both locations on mdaines' version for like four years. It's really frustrating not knowing what changed between old rollup and new rollup, that my old juggle doesn't work anymore.

I was really hoping this fork was going to save me.

[StoneCypher]

I'm fine with maximising the compatibility when we can though.

So it's cool if I need to polyfill, but I don't know what to strip out of the node stuff, and that would involve basically me forking your product

It wasn't clear from the way that was written if you were going to continue here. I still can't get Typescript to compile, despite that this issue is closed, is the thing.