upload icon indicating copy to clipboard operation
upload copied to clipboard

Restructure

Open vixalien opened this issue 1 year ago • 6 comments

While working on adding tests to #9, I realized that the structure of the code made it a bit difficult to add tests effectively. This is hence why I've split the source into different files:

  • base: contains a base class called UploadBase where each uploader will inherit from.
  • browser: contains the UploadBrowser uploader (using XMLHttpRequest)
  • node: contains the UploadNode (using form-data)
  • upload: combines the two into one Upload class that can be used without caring which about the current environment
  • function: creates the upload() function that is based on the Upload class.

This code structure eases DX because each file is doing a specific thing.

I've also thought about moving the UploadBrowser and UploadNode into a separate folder and maybe not export them in index, but I'd need your thoughts on that

vixalien avatar Jul 30 '24 22:07 vixalien

I've also thought about moving the UploadBrowser and UploadNode into a separate folder and maybe not export them in index, but I'd need your thoughts on that

This or finding a better way to do isomorphism - not sure if there's a better alternative to form-data now (maybe node's fetch supports progress now?). Would need to look more into this, the only reason why I chose form-data back then was the need for progress.

Also, I'd only aim to support node.js 18.x+. Maybe even 20.x+ if that would make things easier.

mat-sz avatar Jul 31 '24 16:07 mat-sz

maybe node's fetch supports progress now?

I'm not sure, but I'm quite sure the browser's fetch doesn't support progress, so it's mandatory to still use XMLHttpRequest for the browser, which obviously doesn't exist in node.

Would need to look more into this, the only reason why I chose form-data back then was the need for progress.

I see. I use this package in the browser so I don't really know to be honest.

vixalien avatar Jul 31 '24 16:07 vixalien

I'm quite sure the browser's fetch doesn't support progress

https://stackoverflow.com/questions/35711724/upload-progress-indicators-for-fetch

Could be done with ReadableStream from what I see, you could investigate this further if you'd like, otherwise I can look into this later this week.

mat-sz avatar Jul 31 '24 17:07 mat-sz

That is misleading. When you pass a readable stream to fetch, you'll get notified as the steam is "read". When some data is read of a stream does not necessarily mean that it has been uploaded.

Anyway, I think the best course of action would be to review and land this, then move to other libraries/approaches later i.e. after the restructure.

vixalien avatar Jul 31 '24 18:07 vixalien

Agreed, let's do that then.

TBH, I'm not sure whether maintaining the node.js logic is worthwhile anymore; seems like most uses for this would be on the browser side anyway.

mat-sz avatar Jul 31 '24 18:07 mat-sz

Agreed, let's do that then.

Alright, I'll work on fixing CI

seems like most uses for this would be on the browser side anyway.

Agreed

TBH, I'm not sure whether maintaining the node.js logic is worthwhile anymore;

I still think keeping node support can be useful, though.

vixalien avatar Jul 31 '24 19:07 vixalien