New node packages ship TS source files

[jakebailey]

Describe the bug

I'm working on upgrading Pylance to use version 1.8.0; we use the onnxruntime binding for IntelliCode support.

I noticed when trying to move to the new package that the original TS source is getting shipped. I think this might have been an oversight; the types are provided via the d.ts files, and it's the js files that should be in the final npm package, not the original source. AFAIK, most TS libraries have their source in a src directory (which is npmignore'd), then compile to another directory, which avoids this problem. onnxruntime does not, and at some point during the package move, the ts files in lib must have become unignored.

This seems to cause ts-loader (used in Webpack to build TS code) to ignore the shipped .js files altogether, as it's resolving to the .ts file first, then recompiling it. We have a webpack loader plugin in place to manage loading the multiple .node bindings (for cross-platform support), and that's how I noticed that the js files were no longer being loaded.

Urgency

No major urgency at the moment, 1.7.0 is working fine. I can technically work around the issue (that a file I override to manage the bindings is not a js file), but I don't think it makes sense for downstream projects to be recompiling the library during their builds.

System information

(Not really relevant to this issue.)

To Reproduce

Do npm install onnxruntime-node, then look in node_modules/onnxruntime-core/lib; the ts files are present. I can give you a larger webpack example if needed.

Expected behavior

No source files are shipped

Screenshots
If applicable, add screenshots to help explain your problem.

Additional context
Add any other context about the problem here. If the issue is about a particular model, please share the model details as well to facilitate debugging.

[fs-eire]

@jakebailey thank you for the feedback.

I didn't use ts-loader for Node.js before. For web, to generate bundles it is usually OK to recompile the typescript from dependencies, as the target library may have different output settings (es5/es6/es7, umd/mjs), so sometimes recompiling from source typescript code is better than consuming the bundle.min.js. Usually this does not cause any error, the only disadvantage is a longer build time.

Does the typescript code in the NPM package causes ts-loader to behave unexpectedly (like fail to load the node binding)? If so, please share me the example so that I can take a look into the issue.

[jakebailey]

is usually OK to recompile the typescript from dependencies, as the target library may have different output settings (es5/es6/es7, umd/mjs), so sometimes recompiling from source typescript code is better than consuming the bundle.min.js.

This isn't the way that I've seen libraries do it, at least not so far. I don't know of any library that expects ts-loader or another bundler to compile it when producing the bundle; bundlers (and TSC itself) are typically able to create the right shims to allow interop.

Does the typescript code in the NPM package causes ts-loader to behave unexpectedly (like fail to load the node binding)? If so, please share me the example so that I can take a look into the issue.

My case is unique; in order to support using onnxruntime on all three supported platforms in a single bundle, I replace binding.js at webpack load time with a version that can point to the right .node file (downloaded into the output for bundling), rather than using webpack's own node loader to grab what's in node_modules. If we were to use the loader the normal way, we would only be packaging the version of the onnxruntime for the platform that happened to do the bundle.

I noticed this issue because binding.js was no longer getting loaded and webpack was throwing on loading the .node files; ts-loader was instead choosing to compile the code out of node_modules (the file I was overriding was no longer being used), which is why I noticed this discrepancy.

I will test to see how it works when I tell it to replace the ts file (or, remove my workaround temporarily), but this does go against ts-loader's default configuration and some info on the TS tracker. The fact that it works in some scenarios sounds like a fluke. See:

https://github.com/TypeStrong/ts-loader#allowtsinnodemodules

microsoft/TypeScript#12358

[fs-eire]

fix is done and a dev version package is published onnxruntime-node@1.8.0-dev.20210608.0.

[jakebailey]

I tried it out, but it appears as though the common package is not quite right:

ERROR in ../../node_modules/onnxruntime-common/lib/inference-session.ts
Module build failed (from ../../node_modules/ts-loader/index.js):
Error: TypeScript emitted no output for /home/jake/work/pyrx-yarn/node_modules/onnxruntime-common/lib/inference-session.ts. By default, ts-loader will not compile .ts files in node_modules.
You should not need to recompile .ts files there, but if you really want to, use the allowTsInNodeModules option.
See: https://github.com/Microsoft/TypeScript/issues/12358
    at makeSourceMapAndFinish (/home/jake/work/pyrx-yarn/node_modules/ts-loader/dist/index.js:52:18)
    at successLoader (/home/jake/work/pyrx-yarn/node_modules/ts-loader/dist/index.js:39:5)
    at Object.loader (/home/jake/work/pyrx-yarn/node_modules/ts-loader/dist/index.js:22:5)

ERROR in ../../node_modules/onnxruntime-common/lib/onnx-value.ts
Module build failed (from ../../node_modules/ts-loader/index.js):
Error: TypeScript emitted no output for /home/jake/work/pyrx-yarn/node_modules/onnxruntime-common/lib/onnx-value.ts. By default, ts-loader will not compile .ts files in node_modules.
You should not need to recompile .ts files there, but if you really want to, use the allowTsInNodeModules option.
See: https://github.com/Microsoft/TypeScript/issues/12358
    at makeSourceMapAndFinish (/home/jake/work/pyrx-yarn/node_modules/ts-loader/dist/index.js:52:18)
    at successLoader (/home/jake/work/pyrx-yarn/node_modules/ts-loader/dist/index.js:39:5)
    at Object.loader (/home/jake/work/pyrx-yarn/node_modules/ts-loader/dist/index.js:22:5)

ERROR in ../../node_modules/onnxruntime-common/lib/tensor.ts
Module build failed (from ../../node_modules/ts-loader/index.js):
Error: TypeScript emitted no output for /home/jake/work/pyrx-yarn/node_modules/onnxruntime-common/lib/tensor.ts. By default, ts-loader will not compile .ts files in node_modules.
You should not need to recompile .ts files there, but if you really want to, use the allowTsInNodeModules option.
See: https://github.com/Microsoft/TypeScript/issues/12358
    at makeSourceMapAndFinish (/home/jake/work/pyrx-yarn/node_modules/ts-loader/dist/index.js:52:18)
    at successLoader (/home/jake/work/pyrx-yarn/node_modules/ts-loader/dist/index.js:39:5)
    at Object.loader (/home/jake/work/pyrx-yarn/node_modules/ts-loader/dist/index.js:22:5)

[jakebailey]

Yeah, it's still distributed as it was before.

[jakebailey]

This is still a problem, even moreso in the web package where I get a pretty significant number of errors (which I'm now trying out as I'm hoping to drop needing any of the node bindings in favor of WASM).