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

Start converting Koala into a subclass of Koa #1

Merged
merged 2 commits into from
Sep 13, 2017

Conversation

nickserv
Copy link

@nickserv nickserv commented Sep 6, 2017

I had some old code converting koala into a class (so it would have the same usage as Koa 2 which is a class). Since it would have conflicted with koajs#47 I decided to rebase onto it so I could show you this, and you can optionally merge this into your pull request branch if you like the syntax.

Note that we would at least need to make documentation changes in the readme. Tests should have the same failures, as far as I'm aware this doesn't introduce any regressions as long as you replace koala() with new Koala().

@nfantone
Copy link

nfantone commented Sep 6, 2017

I'm going to say 👎 to this 😞 . Might be an unpopular opinion, but I really don't like the idea of exposing classes as main API. Factory functions do an arguably better job, are cleaner and don't come with any of the drawbacks.

I'm up for favoring functions and composition (the bread and butter of JS) vs. classes and inheritance (not even a JS feature).

@nickserv
Copy link
Author

nickserv commented Sep 6, 2017

The main reason I would like a class export is that Koa is a class export, and since Koala had exactly the same usage for Koa 1 I feel like it would be confusing to keep the Koa 1 style function export while using Koa 2. In other words, I would rather have usage as close as possible to Koa. While I personally disagree with how Koa switched to a class export, I would rather propose a change upstream than make one here. We would also have to reword the the docs that say "simply replace Koa with Koala" because you couldn't instantiate a constructor in the same way, and having this slightly different API might cause issues with upgrading to future Koa APIs in the future.

This being said, I'm a big fan of functional composition myself. In my opinion, object oriented programming does not hinder pure functional programming in JavaScript, I think the issue is with inheritance specifically. Ideally I would personally like to keep the function export (for API compatability) but try to use composition over inheritance, though in this case it seemed like using inheritance made the implementation much simpler.

Anyway, thanks for your feedback, I'm still open to discussion on this and we don't necessarily need to merge this. It was more of an experiment with adapting our API to more closely match Koa 2's, though I would still like to try to find other ways to make the class export easier for us to maintain if possible.

@doug-wade
Copy link
Owner

I like offering Koala as a drop-in replacement for Koa. I'll take a closer look this evening.

@nickserv
Copy link
Author

Thanks. The change isn't too complex, it just has kind of a repetitive diff. To summarize, koala is now a subclass of Koa that is the app itself instead of a function that returns the app. The majority of the koala function has been moved to Koala's constructor with the app object being replaced with this. I updated the tests but we will still need to update the documentation.

@doug-wade doug-wade merged commit 2a5255c into doug-wade:fix-17 Sep 13, 2017
@nickserv nickserv deleted the fix-17-with-class branch September 13, 2017 06:39
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.

3 participants