Skip to content
This repository has been archived by the owner on Apr 29, 2020. It is now read-only.

chore: refactor to async/await #17

Merged
merged 1 commit into from
Aug 27, 2019
Merged

chore: refactor to async/await #17

merged 1 commit into from
Aug 27, 2019

Conversation

achingbrain
Copy link
Collaborator

BREAKING CHANGE: This module used to export a class that extended EventEmitter, now it exports a function that returns an async iterable.

I also updated the deps to use the latest http api, though it's removed the ability to add whole paths at once, along with some special logic to handle symlinks. The Dicer module that this module depends on will still emit events for when it encounters symlinks so I left the handlers in though am unsure if we actually use them.

README.md Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/parser.js Outdated Show resolved Hide resolved
src/parser.js Outdated

if (isDirectory(partHeader.mime)) {
// consume data from stream so we move on to the next header/part
part.on('data', () => {})
Copy link

Choose a reason for hiding this comment

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

It looks like we're calling part.on('data', () => {}) for every header

src/parser.js Outdated

if (partHeader.mime === applicationSymlink) {
// consume data from stream so we move on to the next header/part
part.on('data', () => {})
Copy link

Choose a reason for hiding this comment

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

Same here, should part.on('data') only be called once rather than for every header?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could be doing it wrong but in my testing I noticed that setting up a noop for the data event for all headers caused data to start being read before the calling code had a chance to set a listener up for data from part.content.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually I think there might be a better way, will update this PR today or tomorrow.

package.json Outdated Show resolved Hide resolved
test/parser.spec.js Outdated Show resolved Hide resolved
@achingbrain achingbrain force-pushed the async-await branch 3 times, most recently from bbce927 to 77438e7 Compare August 16, 2019 17:52
Copy link

@dirkmc dirkmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

BREAKING CHANGE: This module used to export a class that extended EventEmitter,
now it exports a function that returns an async iterable.

I also updated the deps to use the latest http api, though it's removed
the ability to add whole paths at once, along with some special logic
to handle symlinks.  The `Dicer` module that this module depends on
will still emit events for when it encounters symlinks so I left the
handlers in though am unsure if we actually use them.
@alanshaw
Copy link

@hugomrdias can has merge and release please 🙏

@hugomrdias
Copy link
Contributor

The Dicer module that this module depends on will still emit events for when it encounters symlinks so I left the handlers in though am unsure if we actually use them.

But dicer was removed, plus now we have a dep it-multipart pointing to this repo.
@achingbrain can you clarify this situation, im probably missing something.

@achingbrain
Copy link
Collaborator Author

Sorry, I should have updated the message above.

I was turning Dicer into an async iterable with the event-iterator module but I kept seeing the (node) warning: possible EventEmitter memory leak detected. 11 listeners added. message.

Because of the range of events Dicer emits (instead of just data, error, etc) and the semantics of consuming message bodies, trying to wrap it in something that turns it into an aync iterable was proving too error prone so I implemented it-multipart as a pure async iterable multipart message parser and used that instead.

@hugomrdias hugomrdias merged commit 55d926e into master Aug 27, 2019
@hugomrdias hugomrdias deleted the async-await branch August 27, 2019 15:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants