Skip to content

docs: move data.fields line in README #320

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 8 commits into from
Jan 20, 2022
Merged

Conversation

frapan
Copy link
Contributor

@frapan frapan commented Jan 19, 2022

Checklist

@frapan
Copy link
Contributor Author

frapan commented Jan 19, 2022

Fixes #252

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

(available only AFTER the stream is consumed)
The above sentence is not always true. I do not want to give wrong explanation to the client just to reduce the issue.

Instead, I would like someone to explain more on how stream or busboy operate and create a Q&A section.

e.g.
busboy consume the multipart in serial order (stream), the order of form field is VERY IMPORTANT to how fastify-multipart can display the fields to you. We would recommend you to place the value fields first before any of the file fields. It can ensure your fields will be accessible before start consuming any file.

Copy link
Member

@darkgl0w darkgl0w left a comment

Choose a reason for hiding this comment

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

Note: if the file stream that is provided by data.file is not consumed, like in the example below with the usage of pump, the promise will not be fulfilled at the end of the multipart processing. This behavior is inherited from @fastify/busboy.

There is a note about this in the README.md just after the example

I would suggest that you add another note with the explaining sentence proposed by @climba03003 instead.

@frapan
Copy link
Contributor Author

frapan commented Jan 19, 2022

Thank you for your explanations.

I reverted to the original file and added the note of @climba03003 with a small addition:

Note about data.fields: busboy consumes the multipart in serial order (stream). Therefore, the order of form fields is VERY IMPORTANT to how fastify-multipart can display the fields to you.
We would recommend you to place the value fields first before any of the file fields.
It can ensure your fields will be accessible before start consuming any file.
If you cannot control the order of the placed fields, be sure to read data.fields AFTER consuming the stream, or it will contain only the fields parsed at that moment.

I also realized that the following note was duplicated:

Note: if the file stream that is provided by data.file is not consumed, like in the example below with the usage of pump, the promise will not be fulfilled at the end of the multipart processing. This behavior is inherited from @fastify/busboy.

So I deleted the one that was in the wrong (imho) position.

What do you think?

@mccare
Copy link
Contributor

mccare commented Jan 19, 2022

On a related note, the documentation here https://github.com/fastify/busboy#busboy-special-events mentions that you can also just use file.resume(). If that is correct, I would also add it to the documentation here. WDYT?

@climba03003
Copy link
Member

On a related note, the documentation here https://github.com/fastify/busboy#busboy-special-events mentions that you can also just use file.resume(). If that is correct, I would also add it to the documentation here. WDYT?

I think it is a separate issue.

@climba03003 climba03003 requested a review from Fdawgs January 20, 2022 02:35
@climba03003
Copy link
Member

Can you kindly help to check the sentence for us?
cc @Fdawgs

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@darkgl0w darkgl0w left a comment

Choose a reason for hiding this comment

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

LGTM with code review suggestions applied.

mcollina and others added 3 commits January 20, 2022 18:41
Co-authored-by: Frazer Smith <frazer.dev@outlook.com>
Co-authored-by: Frazer Smith <frazer.dev@outlook.com>
Co-authored-by: Frazer Smith <frazer.dev@outlook.com>
@mcollina
Copy link
Member

@Fdawgs are you ok now?

@Fdawgs
Copy link
Member

Fdawgs commented Jan 20, 2022

@Fdawgs are you ok now?

All gravy now

@mcollina mcollina merged commit 4cffc6b into fastify:master Jan 20, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants