Skip to content
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

Full base Implementation of node-disks #2

Open
wants to merge 52 commits into
base: upstream-base
from

Conversation

Projects
None yet
2 participants
@bericp1
Copy link
Member

bericp1 commented Apr 12, 2019

See also Carimus/node-uploads#2.

This PR is a merge of master (what's released onto npm right now) into a temporary branch upstream-base which represents essentially the starting point for this PR.

As such, this PR contains the entire functional implementation of node-uploads up to this point (v1.11.0).

bericp1 added some commits Mar 29, 2019

feat(*): import all of the logic from rigpark-api and do js -> ts 💌
Involved reorganizing and defining types out, mostly just converting the jsDoc to typescript types.
Still need tests and docs.
feat(drivers): add memory driver
Added memfs which provides a module that has an API identical to node's `fs` but operates on an in
memory filesystem.
revert(ci): undo prepack for git install
Due to yarnpkg/yarn#5047, prepack doesnt get triggered with yarn so we cant do direct git installs
in a consistent way unfortunately.

bericp1 added some commits Apr 5, 2019

feat(*): isolate MemoryDisks; switch to fs-extras
Large batch of changes including new features and bugfixes:

-   Wrote tests for MemoryDisk
-   Made MemoryDisk use memfs Volume in order to isolate filesystem for seprate instances of the MemoryDisk
-   Fixed FSDisk#delete to calcualte path properly first [major bug missed by lack of tests :(]
-   Support node >= 10 now since we don't rely on fs.readdir's withFileTypes option
-   Support streaming in FSDisk#read by delegating to createReadStream
-   Use fs-extras for LocalDisk to prepare for mkdirp and make other future things easier
Merge pull request #1 from Carimus/feature/temp-and-perm-urls
[Feat] Temporary and Permanent URLs
@KristenKatona
Copy link

KristenKatona left a comment

Just saw a couple TODOs that might need tasks generated if they're still needing to be completed. Too much to look through line by line but scanned through and saw some unit tests and I assume those are passing and provide sufficient coverage (and that we'll test this in our project).

/**
* Pipe a readable stream into a writable stream and resolve once it's completely piped or reject if the pipe fails.
*
* TODO Implement timeout.

This comment has been minimized.

Copy link
@KristenKatona

KristenKatona Apr 16, 2019

Do we need a task to cover implementing a timeout? Is this TODO still relevant?

This comment has been minimized.

Copy link
@bericp1

bericp1 Apr 16, 2019

Author Member

Yep still relevant but nowhere to track it. This isn't crucial to functionality and just a note to a future developer who wants to implement this feature.

* @inheritDoc
*/
public async createWriteStream(): Promise<Writable> {
// TODO Fudge this by returning a Stream wrapper around the putObject stream functionality.

This comment has been minimized.

Copy link
@KristenKatona

KristenKatona Apr 16, 2019

Is this TODO still relevant, do we need a task somewhere to cover it?

This comment has been minimized.

Copy link
@bericp1

bericp1 Apr 16, 2019

Author Member

Yep still relevant but nowhere to track it. This isn't crucial to functionality and just a note to a future developer who wants to implement this feature.

@bericp1

This comment has been minimized.

Copy link
Member Author

bericp1 commented Apr 16, 2019

Hey @KristenKatona !

Yep! You can't actually see in the "All checks have passed section" that every push to this repo runs the tests and will reject if the tests fail. I didn't do a code coverage report but I'd say a good 50% of the code is covered by tests (the remaining untested code is mostly on the s3 side since it's hard to write unit tests against external resources).

I have the meaningful/important TODOs captured in the README which will have to do for now since we don't have a JIRA project to track carimus general / open source stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.