-
Notifications
You must be signed in to change notification settings - Fork 785
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
Es6 rewrite #91
Es6 rewrite #91
Conversation
You can just mark it as draft and concentrate on real info?
This sentence is highly confusing. I don't know what you wanted to say, but please don't overlap PR's with different concerns (linting + ES6 modules + ...), it just makes it cumbersome to review. |
I'm not really familiar with contributions, doing the best I can. I converted the PR to draft for the time being.
Sorry about my poor wording and english. I got it, will be careful. |
Yes, just some explanations why and what you did:
|
And usually that is called the entry point, named It simply exports everything, which I like for debugging. I rewrote @do-me's SemanticFinder into ES6 using ES6 transformers.js and it just makes it nice to quickly test around with: |
Can you also rewrite: if (global.ReadableStream === undefined && typeof process !== 'undefined') {
try {
// @ts-ignore
global.ReadableStream = (await import('node:stream/web')).ReadableStream; // ReadableStream is not a global with Node 16
} catch (err) {
console.warn("ReadableStream not defined and unable to import from node:stream/web");
}
} into code which works in a browser + node at the same time (simply replacing if (!globalThis.ReadableStream && typeof process !== 'undefined') {
try {
// @ts-ignore
globalThis.ReadableStream = (await import('node:stream/web')).ReadableStream; // ReadableStream is not a global with Node 16
} catch (err) {
console.warn("ReadableStream not defined and unable to import from node:stream/web");
}
} This kind of code is also called a polyfill and is often separated into e.g. import 'polyfills/ReadableStream.js';
export * from './tokenizers.js';
export * from './models.js';
export * from './processors.js';
export * from './env.js';
export * from './pipelines.js';
export * from './tensor_utils.js'; |
@takoyaro Did you say you are not afraid to get your hands dirty? Is this still in progress or are you already out? |
I was planning to get back on it this weekend...not sure why you speak to me with this tone but I hope you're ok. |
Thank you! Unfortunately no, I'm not, I'm sitting on a burning stone flying with high-speed through the universe and people have no other issues than "increasing casualties" of each other. I just want to check-in with you since I heard nothing from you for some days and people open issues etc. I'm sorry for the tone and can you forgive me? |
You should be able to continue from the latest version of the docs branch. It seems to work in node.js, but doesn't work in the browser for some reason. Still working that out. It's quite late here, so, I'll try figure it out tomorrow :) |
I'm not sure about the exact problem you run into, but I believe it's because ONNX is not a proper ES6 module: Test code: await import("https://cdnjs.cloudflare.com/ajax/libs/onnxruntime-web/1.14.0/ort.es6.min.js").default However, it should be possible to just import ONNX and after the |
So you're suggesting the problem is occurring in https://github.com/xenova/transformers.js/blob/docs/src/backends/onnx.js? I'll look into it. |
Yep, I just found this issue, they seem to have it on their radar: And their ES6 issue: microsoft/onnxruntime#10913 The ES6 issue has been open for a year, but eventually it should be fixed. Pretty much two options:
@fs-eire Do you happen to have any ETA for ES6 support? I mean... it kinda works, just not in the way ES6 wants it (using Also ES6 standard is to name it |
sorry, I don't have an ETA for ES6 yet. If you would like to contribute, it would be great! The major reason is that we did a lot of "optimizations" for the file size of ort.min.js (UMD), and those are not compatible with webpack with ES6 format. Those including:
I actually worked on this last month, but it turns out to be much more difficult than expected. I may come back some day to revisit this issue. |
Thank you for the quick response! It feels like you are drowning in complexity there. ES6 should be as simple as renaming import * from './tensor.js'; Instead of: import * from './tensor'; This is what every modern browser speaks natively. You can keep the non-standard-ways for CORS etc. to build a bundle while not messing with TypeScript and their AST rewriting first aswell? The interface types just go into Without payment I cannot deep-dive into this, even though I would like to take on this challange. This is also something that probably doesn't take weeks, but maybe two months aswell to get everything right, figured out and tested in multiple browsers/environments etc. |
I did some study and find that it seems no way for me to create a NPM package which allows user to import in both ESM/CJS way: // this is Typescript code
import { a, b, b } from 'onnxruntime-common'; It generates error like this:
This article works for JavaScript but still so far no way to make it works for TypeScript. |
Hi @fs-eire - Yes, this has also been a massive headache for me too (see dev branch). Why doesn't Fortunately, for each of my use-cases, I have found a workaround so that ES6 modules work in node... which was way more difficult the other way around (getting node modules working in ES6). Perhaps @kungfooman can give some advice in this regard, as I am way less experienced haha. |
I already make This is because TypeScript tries to determine whether the current code is CJS/ESM by looking for a nearest "package.json" file, and the solution is to prepare such a file to make it happy. Not sure whether TypeScript will change this behavior in future, but it works for now at least. The change is here: microsoft/onnxruntime#15772, which is far more complicated than just appending the ".js". And I expect doing that for |
From your referenced article: Once you realize that a project/language basically actively blocks you, it's time to identify it as technical debt and get rid of it. I used TypeScript for years, but it became so annoying that I just don't want to touch it anymore (in its "pure" form with all the peculiar design decisions of TS purists who basically dislike JS while barely knowing it). I take the good parts (the type system with turing complete types, the TS language server) and I will probably not convince you, but I think it should be pointed out (before more people apply these kinds of The TypeScript stance doesn't even make sense... they happily introduced enums that transpile to different JS code or public-constructor-assign-AST rewrites, but adding an import-rewrite-feature requested by all kinds of people is ideologically blocked for purity reasons... oh well, glad I'm outta there 😂 |
@kungfooman I know that Typescript has some limitations, but I don't want to jump into the debate of whether or not to use Typescript in my project. The important thing is, as a library developer, I want to make sure my library can be consumed by both JavaScript & Typescript users. Even if I write pure JavaScript with hand-written .d.ts in my package, I still need to deal with the problems and do the same hack to make sure TypeScript users to consume the package happily. |
@fs-eire No problem, lets cease that debate. Do you still run into problems with that PR? I'm not on the right computer for testing your PR, but the way you are using My "exports": {
"import": "./build/playcanvas.mjs/index.mjs",
"require": "./build/playcanvas.js"
}, Testing with node: Funny thing is that it only works for me when the files actually are named <?php
function renameFilesIn($here) {
$files = glob($here);
$n = count($files);
print("rename files in $here n=$n\n");
foreach ($files as $file) {
$mjs_file_name = str_replace(".js", ".mjs", $file);
rename($file, $mjs_file_name);
print("$file -> $mjs_file_name\n");
}
}
renameFilesIn("**/*.js");
renameFilesIn("**/**/*.js");
renameFilesIn("**/**/**/*.js");
renameFilesIn("*/*/*/*/*.js");
renameFilesIn("*/*/*/*/*/*.js");
renameFilesIn("*/*/*/*/*/*/*.js");
?> And the ES5 version is simply generated from the ES6 files (using I'm not sure what your current problems are or if you solved it so far. As test I simply made import { Entity } from "playcanvas";
export function lol() {
const entity = new Entity("test");
console.log("lol entity name:", entity.name);
}
lol(); JS code is simply generated using So if you summarize your current state that would be helpful and I applaud and congratulate you for all of your efforts 👍 💯 🥇 |
using .mjs / .cjs is a good option, but that turns out to be not compatible with some tools (eg. Clang-format). Clang-format only supports The lucky thing is that, the one-liner package.json is working. The PR should be good to consume by TS/JS & CJS/ESM users. |
Glad to hear that 👍 I tested in node and nodes module resolution respects it well. And for browsers people should use import-maps anyway, specifying the ES6 path. Some people of course also prefer to create giant files which contain everything and that should work aswell via
Why do you need |
Thanks to all the work of @xenova in the docs branch, we can load transformers.js and onnxruntime in a browser like this now: <body></body>
<script>
function importFile(content) {
return "data:text/javascript;base64," + btoa(content);
}
const imports = {
"transformers": "./src/transformers.js",
"fs": importFile("export default {};"),
"url": importFile("export default {};"),
"path": importFile("export default {};"),
"stream/web": importFile("export default {};"),
"sharp": importFile("export default {};"),
"onnxruntime-node": importFile("export default {};"),
"onnxruntime-web": importFile(`
await import("https://cdnjs.cloudflare.com/ajax/libs/onnxruntime-web/1.14.0/ort.es6.min.js");
let ONNX = globalThis.ort;
export default ONNX;
export {
ONNX
};
`),
};
const importmap = document.createElement("script");
importmap.type = "importmap";
importmap.textContent = JSON.stringify({imports});
document.body.appendChild(importmap);
</script>
<script type="module">
import * as transformers from "./src/transformers.js";
Object.assign(window, {...transformers});
</script>
<audio id="SPEECH2TEXT_AUDIO" src="./examples/demo-site/assets/audio/jfk.wav" controls="true"></audio> To test, you can just hack code into F12/devtools, e.g. Whisper example: const pipe = await pipeline("automatic-speech-recognition");
const audioCTX = new AudioContext({
sampleRate: 16000
});
const arrayBuffer = await (await fetch(SPEECH2TEXT_AUDIO.currentSrc)).arrayBuffer();
const decoded = await audioCTX.decodeAudioData(arrayBuffer);
const audio = decoded.getChannelData(0);
const result = await pipe(audio);
console.log("result", result); Result: Once onnxruntime is a proper ES6 module, we can simplify the import map code for it, but otherwise it works nice for a quick dev cycle already. |
Great! I also think I've found a way to do it without import maps - will keep you updated :) |
Changes live on main. 👍 |
That's just a WIP, tackling the issues I can, one at a time. Contributions welcome
Also, someone else mentioned using prettier the other day, I'm aware this should be a different PR but I think it'd make contributions easier for everybody, including this one.