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.

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

refactor: es5 > es6 #1

Closed
wants to merge 1 commit into from
Closed

refactor: es5 > es6 #1

wants to merge 1 commit into from

Conversation

henriklundgren
Copy link

Dunno if your implementation is the right way to solve the issue at hand,
but it did indeed clean up my current express application.

I started this pr with the intent to understand what was happening in my own app,
I ended up with refactoring your code so I could read it clearly (es5 hurts my eyes),
I dislike constructor functions, but since you have them I prefer class syntax,
I really detest 4 spaces (2 spaces reads much nicer),
I updated the dependencies,
I added code comments (esdoc style), though prob not complete in any sense.

Most code is left intact, except,
I removed the public callback (it added noise)

Found one fix in private method runInitializers,
it wasn’t looping the initializers, hence couldn’t execute the public configure method when grouped.

Still work to do,

  • Maybe add a editorconfig to enforce your code standards,
  • I dunno if jshint reads es6 or if eslint wouldn’t be a better pick today.
  • Current code coverage implementation doesn’t work and should prob be replaced with something,
    like Istanbul. drop HTMLCov and JSONCov reporters mochajs/mocha#2356
  • Bluebird hijacks the global promise implementation,
    I know there is a release thingy, so yeah.

@jgable
Copy link
Owner

jgable commented Dec 4, 2016

This is such a weird PR, but thanks for putting the time into making it.

As for the fix in runInitializers; in lodash 2.4.1 _.invoke did what _.invokeMap does in the latest version; there was no bug there.

Most of what you are doing here is motion without any progress or improvement. I also love writing ES6 code, now, but when this was written there weren't good transpilation techniques like there are today. That doesn't necessarily mean this needs to be rewritten. IMO, done is better than perfect.

If I accept this pr that re-writes most of the code then it effectively becomes your project and I have no idea who you are or if you'll vanish off the face of the earth tomorrow. Maybe you should start your own version and call it something different; especially if you don't think this implementation is the right way to solve the issue.

@henriklundgren
Copy link
Author

As stated early on,
I was not intending to fix anything nor hunt down bugs in the code,
I only wanted to grasp what was going on in your code.
In the process I rewrote the code to es6.
While I literally only change from constructor functions to class syntax,
require statements to import, and sprinkle some const on top of everything,
I understand your concerns.

In hindsight I might have pushed this to early without properly considering motives for doing so.

Thank you for taking the time to respond.

@jgable
Copy link
Owner

jgable commented Dec 6, 2016 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants