Refactoring and update of dependencied #55
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Because of google recommendation from: > https://google.github.io/styleguide/jsguide.html#features-use-const-and-let Further discussion: > https://stackoverflow.com/questions/41282992/google-js-coding-conventions-the-var-keyword-must-not-be-used
In official node.js docs there is written: Usage of util.inherits() is discouraged. Please use the ES6 class and extends keywords to get language level inheritance support. Also note that the two styles are semantically incompatible. In this commit this problem is solved. Class syntax is used and replacing prototypes. Sources for further reading: > https://nodejs.org/api/util.html#util_util_inherits_constructor_superconstructor > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes > nodejs/node#4179 > https://medium.com/@parsyval/javascript-prototype-vs-class-a7015d5473b
For the purpose of clarify what exactly happens in code, which events are possible, which impossible, and what it does mean there was created list of events. It is recommended to use these constants instead of names of event in stings because it allow to differentiated between events from different sets. For example event 'error' or 'end' can be understood differently in context of imap or notifier.
Instead of simple string in code constants wit events names are applied. It allow to differentiated between events of different objects that can have the same name. It allows to manage names of events independent to logic of code. Sources: Pros of this approach (in php but it desn't matter) > https://pehapkari.cz/blog/2017/07/12/the-bulletproof-event-naming-for-symfony-event-dispatcher/ Example of merge in big js project where constants were used as names of events > mochajs/mocha#3655 Opposed opinion (why constant is bad idea): > https://stackoverflow.com/questions/42713927/why-arent-constants-used-for-events-in-node-js So I invite to the discussion if you think that these changes are bad. Why `require('events')` instead `require('events').EventEmitter`? Both forms works but proposed in this commit is now recommended and agreed in current documentation. Further reading: Edit of first answer there > https://stackoverflow.com/questions/21557369/javascript-requireevents-eventemitter Official docs: > https://nodejs.org/api/events.html Code with backward compatibility > https://github.com/nodejs/node-v0.x-archive/blob/master/lib/events.js
Changes similar to these for Imap and Notifier events.
Not all packages are updated. There is problem with mialparser. I believe it will be solved in further commits. Now there are added comments and problem is highlighted.
qertis
reviewed
Sep 4, 2019
| "imap": "~0.8.9", | ||
| "async": "^2.6.2", | ||
| "debug": "^4.1.1", | ||
| "imap": "~0.8.19", | ||
| "mailparser": "0.6.2" |
There was a problem hiding this comment.
I think that many of packes there was updated from time when I created this pull request.
You are invited add new pull request with updates.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Main goal of this refactoring was understanding error described in this package:
and
imappackageMain change is having all events in constant that are described in comments.
Meaning of error and end or close of different EventEmitter is different.
Main function of this package is mapping between different events so
having the same names was confusing. This problem was not solved and
source of
ECONNRESETis still not found, but this refactor helps to deducethat there are two options:
ECONNRESETin his code - we can describe it in documentationECONNRESET- and this is bug of node