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.

Sign up for GitHub

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

Jump to bottom

Work in progress: Improve filestream #169

Closed
wants to merge 24 commits into from
Closed

Work in progress: Improve filestream #169

wants to merge 24 commits into from

Conversation

pbricout
Copy link
Contributor

@pbricout pbricout commented Apr 16, 2023 •
edited
Loading

New implementation makes completely isomorphic the read/write sync and async implementation, the only difference in the async implementation are the file calls and the await.
The new implementation handles multiple ID3 tags.

Still to do:

  • Improve write implementation
  • Make buffer size an option for write, write test to cover different code path for writeId3TagToFileSync/Async
  • Improve read implementation
  • Refactor common parts of the steam process class
  • Make buffer size an option for read, write test to cover different code path for read
  • Fix temp file generation, actually Date.now() may generate several times the same file especially when tests are ran in parallel and even more if the precision is reduced
  • Something is broken with Node 10.x I must use a feature not available in node 10.x

@pbricout pbricout marked this pull request as draft April 16, 2023 06:56
@pbricout pbricout force-pushed the filestream branch 10 times, most recently from 65d42fd to 3aba7cd Compare April 23, 2023 20:33
pbricout and others added 3 commits April 23, 2023 22:38
@pbricout
Cleanup util-text-encoding
44b5399 
- remove unnecessary string parameter
- move robust conversion table
- improve names
- throws instead of fallback as this is unknown behaviour
  (unless id3 specs document it otherwise)
@pbricout
Validate encoding when reading frame
77697fd
@Zazama
Merge pull request Zazama#171 from pbricout/more-cleanups
5ff0f26 
Validate text encoding and cleanup util-text-encoding
@pbricout pbricout force-pushed the filestream branch 3 times, most recently from 0f97ad4 to 7fa52bb Compare April 25, 2023 19:37
@pbricout pbricout force-pushed the filestream branch 3 times, most recently from 5f58840 to 4d3f1e1 Compare May 7, 2023 23:16
Zazama and others added 10 commits May 8, 2023 01:17
@Zazama @pbricout
Stream files on read instead of fully consuming them
231ec9a
@Zazama @pbricout
Fix fs close bugs and refactor
ba50865
@Zazama @pbricout
Add NodeJS 10 support for file streaming
6ed6c73
@Zazama @pbricout
Add file streaming to write operation
a45ee73
@Zazama @pbricout
eslint fixes
71f1dfe
@Zazama @pbricout
Call callback on success
f7c7047
@pbricout
Improve file-write
1e4668b 
- simplify the code and remove some functions
- remove unnecessary unlink before the rename
  this is acually better as rename is atomic so we don't
  lose the original file if the rename fails
- delete the temp file in the case of error
- factor common code for tmp filename:
  this allows to replace two functions with one
- factor common code in file copy (findId3TagPosition):
  this simplifies the function
- factor common code to remove id3 tag from buffer:
  this allows to replace two functions with one
- simplify function names
- split long lines
- implementation is much shorter
- the sync/async implementations almost only differs with
  this calls to sync/async APIs
@pbricout
Document further simplification to be done
4b7721e
@pbricout
Remove the use of "tmp" library
6fa629e
@pbricout
Cleanup the API and more
011083b 
- remove support for function which do not make sense for:
  create, read, write, update, promises
- clean up return types and throws in case of error
  (create with callback for example)
pbricout added 10 commits May 8, 2023 01:17
@pbricout
Add write option file buffer size
4a1b7df 
- introduce WriteOptions type
- implements a new write API proposal
- update tests to use file buffer size options
  (more tests to do to improve coverage)
@pbricout
Add some failing write test cases
461f08e
@pbricout
Add a getId3Tag() util function
e9d6a53 
Returning this information is cheap and will simplify code
in other places.
@pbricout
Fix write for ID3 id spanning across two read access
5c0af14 
- also simplify some names
@pbricout
Simplify removeId3Tag()
d74697d
@pbricout
Simplify getId3TagBody()
a22a0b1
@pbricout
Rename getId3TagDataFromFileAync()
e00d635
@pbricout
Experimental: Use generator in Id3TagRemover
661adec 
Not really simpler, the only benefit is more scoped `let tag`.
Not really worth the extra complexity.
@pbricout
Simplify file write
b0c5c71
@pbricout
wip: Refactor file-read
b1d7779
@pbricout pbricout force-pushed the filestream branch 2 times, most recently from 4783977 to 758fff0 Compare May 7, 2023 23:27
@pbricout pbricout changed the title Work in progress: filestream Work in progress: Improve filestream May 7, 2023
@pbricout
Copy link
Contributor Author

pbricout commented May 7, 2023

Closing, will use a different branch name to make it easier to work on this.

@pbricout pbricout closed this May 7, 2023
@pbricout pbricout deleted the filestream branch May 7, 2023 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pbricout @Zazama