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

Upload dir async #855

Merged
merged 32 commits into from
Jun 16, 2023
Merged

Upload dir async #855

merged 32 commits into from
Jun 16, 2023

Conversation

GrosSacASac
Copy link
Contributor

@GrosSacASac GrosSacASac commented May 18, 2022 •
edited
Loading

For some reason parse callback is never called, I did not yet figure out why

Context:

#853

@GrosSacASac GrosSacASac mentioned this pull request May 18, 2022
Closed
@GrosSacASac
Copy link
Contributor Author

@wbt and @Akxe have another look, it works now

The thing I forgot initially when moving to async, is I have to await for a directory to be created and then let the actual file that has that directory destination continue

@wbt I would be curious what the code you made looked like

tunnckoCore
tunnckoCore reviewed Jun 16, 2022
View reviewed changes
src/Formidable.js Outdated Show resolved Hide resolved
tunnckoCore
tunnckoCore previously approved these changes Jun 16, 2022
View reviewed changes
Copy link
Member

@tunnckoCore tunnckoCore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good.

Akxe
Akxe reviewed Jun 17, 2022
View reviewed changes
src/Formidable.js Show resolved Hide resolved
src/plugins/multipart.js Show resolved Hide resolved
@GrosSacASac GrosSacASac marked this pull request as ready for review July 13, 2022 15:50
@wbt
Copy link
Collaborator

wbt commented Jul 14, 2022 •
edited
Loading

@wbt I would be curious what the code you made looked like

It had client-side recursive iteration that sent several requests to the server, one for each folder creation and file upload. The server side did path validation (including checking for attempts to walk up the directory tree) etc.

@GrosSacASac
Copy link
Contributor Author

@wbt thanks for sharing

@wbt
Copy link
Collaborator

wbt commented Jul 18, 2022

It might be helpful to add some documentation updates with this PR too.

@GrosSacASac
Copy link
Contributor Author

It might be helpful to add some documentation updates with this PR too.

@wbt Added a note in the readme

Published https://www.npmjs.com/package/@grossacasacs/formidable here so I can test it for a while

@GrosSacASac
Copy link
Contributor Author

@tommyhtran please help to review this PR

@GrosSacASac GrosSacASac mentioned this pull request Dec 1, 2022
Closed
sboudouk
sboudouk reviewed Dec 2, 2022
View reviewed changes
Copy link

@sboudouk sboudouk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks.

@GrosSacASac GrosSacASac mentioned this pull request Dec 5, 2022
Open
@GrosSacASac
Copy link
Contributor Author

Thanks for the review, I will add a few more tests then merge

sboudouk
sboudouk reviewed Mar 22, 2023
View reviewed changes
Copy link

@sboudouk sboudouk left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting feature and code LGTM, when do you expect to make this live ?

@GrosSacASac
tests: update sha1 (added linebreak)
1c977e5
@GrosSacASac
test: update special chars
168734f 
we decode # encoded things now ?
@GrosSacASac
test: force carriage return and use fetch
d1c63d5
@GrosSacASac
test: force async,
4766856
@GrosSacASac
test: remove unused
ac4911e
@GrosSacASac
test: move, use node for tests in test-node
3272550
@GrosSacASac
test: try to fix jest error
e280dbf
@GrosSacASac
test: update and fix custom plugin fail
0006b0f
@GrosSacASac
test: disable this test, cannot understand the error
5103354 
    ReferenceError: require is not defined

       8 |   size: 1024,
       9 |   filepath: '/tmp/cat.png',
    > 10 |   name: 'cat.png',
         |       ^
      11 |   type: 'image/png',
      12 |   lastModifiedDate: now,
      13 |   originalFilename: 'cat.png',

      at _getJestObj (test/unit/persistent-file.test.js:10:7)
      at test/unit/persistent-file.test.js:19:1
@GrosSacASac
test: detect problematic test case, comment out (todo)
89c2540
@GrosSacASac
test: add test case for createDirsFromUploads option
39d98d9
@GrosSacASac
test: semicolons and others
af9181e
@GrosSacASac
chore: version and changelog
938d389
@GrosSacASac
feat: test command runs all tests at once
7074ba7
@GrosSacASac
chore: update node version for testing
0d4bc32
@GrosSacASac
chore: up dependencies like in v2
951a94a 
9afd5f8
@GrosSacASac
chore: mark as latest on npm
bc4e236
@GrosSacASac GrosSacASac merged commit c249922 into master Jun 16, 2023
@GrosSacASac GrosSacASac deleted the upload-dir-async branch June 16, 2023 11:08
@GrosSacASac
Copy link
Contributor Author

All the test pass now ;)
But they don't even run in github actions ...
Published as 3.3.2

@GrosSacASac GrosSacASac mentioned this pull request Jun 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sboudouk sboudouk sboudouk left review comments

@Akxe Akxe Akxe left review comments

@tunnckoCore tunnckoCore tunnckoCore left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@GrosSacASac @wbt @Akxe @tunnckoCore @sboudouk