Repository navigation
Notes on/for v3 / v4 #830
Description
Activity
go async/await and promises; possibly drop callback APIs entirely
Maybe do a mix in v3 and in v4 drop callback
Reacted by Charlike Mike ReagentReacted by Charlike Mike ReagentNew
FormidableFile, based onfetch-bloband Web APIFile. Plus, we'll always make temporary files on tmp dir, for the time the process is running. When the process ends, they are cleared (all that is handled byfetch-blob). The user will still be able to to save on disk or to do that file whatever it wants. Something likeopts.saveon Formidable Options, will mean that we will save the data of that temporary created file to the disk on the user-chosen (or default) upload dir.It handles:
- random filename generation (default) with
cuid - file rename function
opts.rename - filename sanitization, ALWAYS, with
filenamifyPath(withopts.replacement: '-'instead of the default!)- whether or not you use custom rename function, the result is always passed to sanitization
- not yet sure if we should do that when renamer function is passed
/** * @typedef {import('filenamify').Options} FilenamifyOptions * @typedef {import('node:path').ParsedPath} ParsedPath * * @typedef FormidableFileOptions * @property {string} [type] - mime type from header * @property {number} [lastModified] - file mtimeMs, default `Date.now()` * @property {number} [size] - file size in bytes * @property {boolean} [randomName] - generate random filename, default `true` using `cuid` * @property {FilenamifyOptions} [filenamify] - options passed to `filenamify` * @property {RenamerFn} [rename] - filename renaming function * @property {{[key: string]: unknown}} [data] - any additional data or way of storing things * * @typedef {(filepath: string, contents: unknown, options: FormidableFileOptions) => string} RenamerFn * must be synchronous function */ import path from 'node:path'; import cuid from 'cuid'; import { filenamifyPath } from 'filenamify'; /** * @typedef {{type?: string, name: string, lastModified: number}} File * standard Web API File */ // @ts-ignore import { File } from 'fetch-blob/file.js'; export class FormidableFile extends File { /** * * @param {string} filepath - filename from header * @param {unknown} contents - any file content value * @param {FormidableFileOptions} [options] */ constructor(filepath, contents, options) { const { file, opts, fp } = normalize(filepath, contents, options); super(contents, file.base, { // @ts-ignore type: opts.mimetype || opts.mime || opts.type, lastModified: opts.lastModified || Date.now(), }); /** * @type {string} - full filepath */ // we assign this here, because to not mess with ParsedPath `file` this.path = fp; // eslint-disable-next-line no-underscore-dangle this._updateProps(file, contents, opts); } /** * * @param {ParsedPath} file - custom file * @param {unknown} contents - any file content value * @param {FormidableFileOptions} [options] */ // eslint-disable-next-line no-underscore-dangle _updateProps(file, contents, options) { // it's already normalized // const opts = normalizeOptions(options); this.basename = file.base; this.dirname = file.dir; this.extname = file.ext; this.stem = file.name; // this.stem == this.name == file.name == basename without extname this.mime = options.type; if (options.size) { // @ts-ignore this.size = options.size; } Reflect.defineProperty(this, 'contents', { value: contents }); Reflect.defineProperty(this, 'options', { value: options }); Reflect.defineProperty(this, 'base', { value: file.base }); Reflect.defineProperty(this, 'dir', { value: file.dir }); Reflect.defineProperty(this, 'ext', { value: file.ext }); Reflect.defineProperty(this, 'data', { value: options.data }); } } /** * * @param {string} filepath - filename from header * @param {unknown} contents - any file content value * @param {FormidableFileOptions} [options] * */ function normalize(filepath, contents, options) { const opts = normalizeOptions(options); /** * @type {string} fp */ const fp = filenamifyPath( // @ts-ignore opts.rename(filepath, contents, opts), opts.filenamify, ); /** * * @type {ParsedPath} file */ const file = path.parse(fp); return { file, opts, fp }; } /** * @param {{[key: string]: unknown} & FormidableFileOptions} [options] * @returns {FormidableFileOptions} */ function normalizeOptions(options = {}) { const opts = { filenamify: { replacement: '-', ...options.filenamify }, randomName: true, data: { ...options.data }, ...options, }; /** * @property {RenamerFn} */ opts.rename = (f, c, o) => { if (typeof opts.rename === 'function') { return opts.rename(f, c, o); } return opts.randomName ? cuid() : f; }; return opts; } /** * examples */ const virtualFile = new FormidableFile( './foo/bar/qu_sas<mail@at.com>zab.gz<script onchange="alert(1)"></script>sax.png', await fs.readFile('./examples/10k.json'), { type: 'application/json', randomName: true, data: { sasa: 1 }, }, ); console.log(virtualFile);
Ideally it will be called from
part.on('data')in the future, but for now we can add it at_newFile, then we can usecreateTemporaryBlobto make that temp file and pass the return to the user.Something like that:
part.on('data', async () => { let file = new FormidableFile(nameFromHeader, dataFromParser, { type: contentTypeFromParser, rename: formidableOptions.rename, }); const blob = await createTemporaryBlob( file.contents, file.type, file.options.signal, ); file.blob = blob; this.emit('file', file, blob); if (formidableOptions.save) { await fs.writeFile(file.path, file.contents); } });
We are using
createTemporaryBlobinstead ofcreateTemporaryFilebecause we already made a file instance of FormidableFile which extendsFile.The reason of having our own File implementation on top, is that it's for useful properties that we need to pass to the users. Plus I always seeing it that way since I got in here in v1. The properties of it is based on enough battle-tested
vfile(remark, rehype, unified) andvinyl(gulp), plus of course the minimalism ofFile.Currently
text(),stream()and those methods, are missing, but we probably should implement them too.The whole thing can be introduced in v3 (and require node 14) and backport to v2 (without the temp file creation thing and without
fetch-blob).The rest of the interesting stuff like iterators and new APIs goes to v4.
- random filename generation (default) with
Okay, so the @jimmywarting's cleaned Formidable parser, which is on the
node-fetchis almost 2x faster than the current one.Based on the
busboys benchmarkbench-multipart-files-100mb-bigthe old one is avg3.7s, now it's1.7swow, didn't know that mine was 2x faster. did you come to any conclusion of why that is?
Not yet. I'm not in parsers and that parser looks like magic to me LOL.
Btw, to make a note: it's faster when use just the class, but not the
toFormData. And when you start putting things on top of all that it gets slower. Busboy v1 really does a great job at the very low level.I'll commit updated benchmarks soon.
fetch-blob@3.2.0 is publised, you can now use
createTemporaryBloborcreateTemporaryFileif you likeReacted by Charlike Mike Reagentbtw, formdata is built into NodeJS core now... maybe you can do a conditional import like use some formdata package like formdata-polyfill (used by node-fetch) or formdata-node (both are spec compatible) if it isn't avalible on global namespace
so something like
const FormData = globalThis.FormData || await import('formdata-polyfill/esm.min.js')
just so happen to know that NodeJS (undici's FormData imp accepts 3th party blobs like fetch-blob as well - not just only instances of globalThis.Blob)fetch-blobis still going to stick around until nodejs/node#37340 has been resolved by nodejs itselfReacted by Charlike Mike ReagentWas a super long time ago since i even used
gulpand i have never heard ofvinylorfilevbefore... honestly just wished they followed Web File IDE structure instead...vinyl should also impl text, arrayBuffer and stream() 👍
Reacted by Charlike Mike Reagentthey followed Web File IDE structure instead
Well, yea. But they predated everything i think, that's why.
I built some
formidable-minion top of yourfetch-blobandformdata-polyfill, you can check it here, it's pretty clean, small and a beauty, haha. ThereFormidableFileis just a Web File, or anyone can just pass the return stadnard FormData to their desired place :Pah.. what a times..
I'm open for ideas and feedback. Not sure how to use these
createTemporary*yet..So my plan is to add a few tests to #855 then merge and publish. This will probably be the last version 3.
And then probably mark 3 as latest. So we can avoid touching v2 like #908 and void having issues that were already fixed in version 3
Then finish #899 which transitions to typescript, so this will be version 4.
Reacted by Charlike Mike ReagentReacted by Charlike Mike Reagent@GrosSacASac Agree.
Seems like people are always just using
latestand don't even read or care about anything else. I'm watching the downloads on all versions, and the majority still are on the v2, after the v2.1 publish they started moving to it, but at a very very slow pace.Note: v3 also need to have CJS. We can use
bundtwhich is a very small and simple regex based converter of import-exports to CJS (does only that), so that we can have both ESM and CJS.Note: v3 also need to have CJS. We can use bundt which is a very small and simple regex based converter of import-exports to CJS (does only that), so that we can have both ESM and CJS.
it could be possible to also just create a simple but small cjs wrapper around ESM since the nature of the parser is async anyways...
// index.cjs module.exports = function (opts) { return { // import the ESM module lazily when needed, and pass the arguments along parse: (...args) => import('./index.js').then(mod => mod.dafault(opts).parse(...args)) } }
Then the code could still be written as ESM and you wouldn't have to duplicate all of your code as a dual cjs/esm package hazard that increases the pkg size?
Reacted by Charlike Mike Reagent@jimmywarting uh, yea. I actually do it similarly here and there. Good point.
Why not merge the TypeScript PR into version 3?
Don't think i have much saying in this... but i don't like TS syntax, it dose not run in browsers and never will.
the ESM support and extension-less paths in TS is a hot garbage atm.
I rather much prefer jsdoc + checkjs / allow js combo.
I much more prefer build-free solutions that just works without wasting lots of time compiling stuff that takes time.you can have equally good type support with plain vanilla javascript. most of the times you don't even need to annotate either javascript or typescript.
let me give you an example. how would you go about usingAudioto calculate the duration of a source? you would need a promise to listen on onloadedmetadata
you may write something like this:function getDruation() { return new Promise<number>((rs) => { const audio = new Audio() audio.preload = 'metadata' audio.src = url audio.load() audio.onloadedmetadata = () => { rs(audio.duration) } }) }
getDruationreturns anumber
but if we rewrite it a little bit with async/awaitasync function getDruation() { const audio = new Audio() audio.preload = 'metadata' audio.src = url audio.load() await new Promise(rs => audio.onloadedmetadata = rs) return audio.duration }
then you suddenly don't need to annotate anything.
there are other more clever ways to have typing without annotating your code.
if you use default parameters in functions then it can also figure out what the types are.@jimmywarting agree. Not fan of TypeScript too. Love the explosion and the attention, and how the ecosystem looks, but gosh I can't stand it to write a whole codebase in TS. I think if it's so needed we can expose just a separate d.ts, and when the codebase is drastically simplified it won't be that needed too.
As I said at #899: "I'll review it at a later point, but I don't think a review is much needed. Maybe the only thing that I could ask is to try with separate file approach - e.g. just a typings file that we will distribute with the package. Just to see how it looks like."
I can understand a few of the frustrations with TypeScript, but maybe it would help to view it as a necessary evil.
As it is, I already found a few coding mistakes in the process of converting the codebase to TS. It sounds like the drawback here is having a compilation step and being forced to write code a little differently. IMO, compilation isn’t a big deal. Taking away the ability to write code more freely is what we need consensus on.
The alternative to the PR is probably going to be the status quo, type-less development in JS and a .d.ts file that may fall behind on any given commit.
Reacted by Charlike Mike Reagent@tommyhtran agree.
But I'm coming from that that the parser could be hard to be typed, and really there's not much need for that - it's an internal.
The rest is just a big bloat around the parser which can be simplified dramatically just like I did with formidable-mini (i'm playing around), which is around the same parser, but the rest on top of it is just a single simple class which won't be that hard to be written in TypeScript - not like the current one.The thing is that we obviously (based on downloads stats of older versions) should focus on legacy and backporting things.. which drives out the motivation to work at all. In 2023 I'll figure out how to make users to migrate or upgrade, and why not we can get paid for helping and consulting. I'm very excited about v3, v4 and formidable-mini, but..
Ok lets have a vote (react with one)
Continue with current situation (types in a separate project) ❤️
Add type defintion side by side to original source files (.js files + .d.ts files) 🚀
Convert project to typescript and have type definition in the source files, use a compiler to have js files on publish 😄
Reacted by Aashiq Dheeraj, Hichem, Dmytro Tiapukhin, GrosSacASacs, Lynx, Sebastian and Charlike Mike ReagentReacted by GrosSacASacs and Charlike Mike ReagentReacted by Jimmy Wärting, Aashiq Dheeraj, Andrei Lahunou, Dmytro Tiapukhin, GrosSacASacs and Charlike Mike Reagent- pinned this issue
on Mar 10, 2024 @GrosSacASac i'll move
formidable-minias a separate repo in the org.Currently it is on https://github.com/tunnckoCore/opensource/tree/master/modules/formidable-mini.
We can start moving tests there, and maybe tag it as v4? It's already API compatible kind of, without the file size options and stuff, but could be used for migrating users. And we can type it too.
Tweaked version of the multipart parser & the multipart plugin (not depending on
thisor tihng sfrom the upstream Formidable class).Seems like it is performing prettty well, STILL, given that we drop the bloat of file handling and old streams handling, and having a main Formidable class.
#996 and the trimmed version can be found on the @mjackson's multipart-parser bench folder
v4 published on
nextnpm dist-tagDocs and basic examples at: https://www.npmjs.com/package/formidable/v/4.0.0-rc.3
it's a fork of the
@mjackson/multipart-parser, because A) we can't build on top directly for now, and B) it's unbeatable. The limit options have to be inside the parser to be able to skip calling the handler for that part if it's above the limits.api
import { parseMultipartRequest, parseMultipart, formidableDefaultOptions, FormidableError, type FormidableOptions, type FormidablePart, } from 'formidable'; // for node 18+, Bun/Deno/Cloudflare // For node.js v14 and v16 compat // import { parseMultipartRequest } from 'formidable/node14'; // import { parseMultipartRequest } from 'formidable/node16'; try { await parseMultipartRequest(req /* Request*/, options, async (part: FormidablePart) => { // part.type - part content type, media type (no charsets) // ==== Headers ==== // part.headers - part headers, parsed into type-safe SuperHeaders object // part.headers.contentType.mediaType - part content media type // part.headers.contentType.charset - part content charset // part.headers.contentDisposition.name // part.headers.contentDisposition.filename // part.headers.contentDisposition.preferredFilename // part.headers.contentDisposition.type - attachment or inline or form-data // ==== Streaming ==== // await part.stream() - no buffering, async iterable, useful to use in `for await (const chunk of part.stream())` // ==== Buffering ==== // await part.text(failSafe?) - buffer into string, pass failSafe = true to avoid crashing // await part.bytes() - buffer into Uint8Array bytes // await part.arrayBuffer() - buffer into ArrayBuffer // await part.json() - buffer into JSON object, pass failSafe = true to avoid crashing // ==== Utils ==== // part.toString() - string representing the state of properties (name, filename, type, headers) // part.toObject() - the core of `toString()`, returns an object with the properties (name, filename, type, headers) // part.isFile() - check if the part is a file if (part.isFile()) { console.log('file', part.name, part.filename, part.toString()); } else { // part.text() on field gets the input's value console.log('field', part.name, await part.text()); } }); } catch (er: FormidableError) { switch (er.code) { case 'ERR_INVALID_INPUT': console.error(er.message); break; case 'ERR_BODY_CONSUMED': console.error(er.message); break; case 'ERR_FAILED_TO_PARSE_TEXT': console.error(er.message); break; case 'ERR_FAILED_TO_PARSE_JSON': console.error(er.message); break; case 'ERR_NO_BOUNDARY': console.error(er.message); break; case 'ERR_MAX_FILENAME_SIZE': console.error(er.message); break; case 'ERR_MAX_FILE_SIZE': console.error(er.message); break; case 'ERR_MAX_FILE_KEY_SIZE': console.error(er.message); break; case 'ERR_MAX_FIELD_SIZE': console.error(er.message); break; case 'ERR_MAX_FIELD_KEY_SIZE': console.error(er.message); break; case 'ERR_MAX_HEADER_SIZE': console.error(er.message); break; case 'ERR_MAX_HEADER_KEY_SIZE': console.error(er.message); break; case 'ERR_MAX_HEADER_VALUE_SIZE': console.error(er.message); break; case 'ERR_MAX_ALL_HEADERS_SIZE': console.error(er.message); break; } }
Maybe we should discuss the API changes.
If the API is too different from previous version then, let's not repeat the mistake of Angular.
Kinda agree but also not.
The API isn't what's important. The problem is the unnecessary bloat that causes all the edge cases and problems and the majority of what actual users are facing.
The biggest feature of formidable is speed, streaming, and limit options. As the API.. sure we still can expose all that as just
parse. But the best would be to NOT actually use that but the actual streams/iterators for best performance and ZERO buffering.The v4 API is modern way of streaming, a whole lot easier and less crashing. It's normal to continue following that streaming path.
I've pinged a bunch of projects to migrate to at least v3, some to try v4 directly. Most are facing weird problems purely based on the bloat on top of the parser. The parser is not even touched for a decade.
I've rewrote it from scratch at least 7 times just in the last month. I never realized how weird and inefficient it is. So it turned out it's really close to impossible to beat the MJackson's one in benchmarks. One of the tries was pretty close but once I've added features and options we need, it degrades a bit which is normal. So that's the one in v4 in the
nextdist-tag.Not to mention the most famous problem everyone is facing is the one that they gotta disable the frameworks body parser cuz we are touching a bunch the request. The v4 is just multipart. It's absolutely unnecessary to touch the other kind of formats because the language evolved, people got used enough to modern terms and APIs like File and FormData.
I really don't want to touch v3. I want to be stable, fast and serious solution for the modern workflow, without the bloat, without making people wonder why the heck it doesn't work with Nextjs, or why this event wasn't fire, or why this closed, or.. aaggh, or similar sort of problems. Heck, why not build a small ecosystem around us, embed us somewhere more, build micro service to fund further development and education or help for teams.
That's the unlock. Not sitting on the old forgotten APIs, streams shenanigans, and literally unfixable issues and unbackportable features.
The v3 would go into LTS, it would still be here in the monorepo, but I'll not touch it.
Duh.. thought that's going to be a short one. haha.
That's my plan. Sudden, but.. 🤷♂️ I really see a future, and opportunity.
workspaces/
misc
(turborepo /nx.dev + lerna ^5.1 + yarn workspaces)- Hela v6latestas masterFile