-
Notifications
You must be signed in to change notification settings - Fork 1k
[v4] Switch build system to use esbuild #1466
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
base: v4
Are you sure you want to change the base?
Conversation
|
Amazing -- I will be testing this out in the next few days! 😍 |
xenova
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Good start 👍
| "esbuild": "^0.27.0", | ||
| "onnxruntime-node": "1.24.0-dev.20251104-75d35474d5", | ||
| "onnxruntime-web": "1.24.0-dev.20251104-75d35474d5", | ||
| "rimraf": "^6.1.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be devDependencies
| console.log(`\nBuilding ${regularFile}...`); | ||
| await esbuild({ | ||
| bundle: true, | ||
| treeShaking: true, | ||
| logLevel: "warning", | ||
| entryPoints: [path.join(rootDir, "src/transformers.js")], | ||
| platform, | ||
| format, | ||
| outfile: path.join(outdir, regularFile), | ||
| sourcemap: true, | ||
| external: externalModules, | ||
| plugins, | ||
| logOverride: { | ||
| // Suppress import.meta warning for CJS builds - it's handled gracefully in the code | ||
| "empty-import-meta": "silent", | ||
| }, | ||
| }); | ||
| reportSize(path.join(outdir, regularFile)); | ||
|
|
||
| console.log(`\nBuilding ${minFile}...`); | ||
| await esbuild({ | ||
| bundle: true, | ||
| treeShaking: true, | ||
| logLevel: "warning", | ||
| entryPoints: [path.join(rootDir, "src/transformers.js")], | ||
| platform, | ||
| format, | ||
| outfile: path.join(outdir, minFile), | ||
| sourcemap: true, | ||
| minify: true, | ||
| external: externalModules, | ||
| plugins, | ||
| legalComments: "none", | ||
| logOverride: { | ||
| // Suppress import.meta warning for CJS builds - it's handled gracefully in the code | ||
| "empty-import-meta": "silent", | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minimize duplication by defining a list of defaults which we unpack unto each config.
|
|
||
| try { | ||
| console.log("=== CLEAN ==="); | ||
| execSync(`rimraf ${DIST_FOLDER}`, { stdio: "inherit" }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we are going to call rimraf using execSync, we might as well just use native fs functions to clear the dist directory. -1 dependency :)
| export const NODE_EXTERNAL_MODULES = [ | ||
| "onnxruntime-common", | ||
| "onnxruntime-node", | ||
| "sharp", | ||
| "node:fs", | ||
| "node:path", | ||
| "node:url", | ||
| ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the node:... list may grow in future. Is there a way to specify a pattern like /^node:.*/?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(this may only be possible in the section of code where we use it)
| contents: ` | ||
| const noop = () => {}; | ||
| const emptyObj = {}; | ||
| export default emptyObj; | ||
| export const Readable = { fromWeb: noop }; | ||
| export const pipeline = noop; | ||
| export const createWriteStream = noop; | ||
| export const createReadStream = noop; | ||
| ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this be added one, or for each module we will ignore? 👀
| const ORT_JSEP_FILE = 'ort-wasm-simd-threaded.jsep.mjs'; | ||
| const ORT_BUNDLE_FILE = 'ort.bundle.min.mjs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[WIP] How do we ensure this is bundled with the main library, enabling #1471
| } | ||
|
|
||
| console.log("=== CLEAN ==="); | ||
| execSync(`rimraf ${outdir}`, { stdio: "inherit" }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above.
| logOverride: { | ||
| "empty-import-meta": "silent", | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above RE: duplication.
Maybe we can also consolidate dev and build logic to reduce duplication
No description provided.