References to "this" causing issues

[amaschas]

koa-router version: 7.4.0

koa version: 2.7.0

Code sample:

const router = new Router();

const AppRoutes = [
    {
        path: '/',
        method: 'get',
        action: indexController,
    }
]

AppRoutes.forEach(route => {
  router[route.method](route.path, route.action);
});

Expected Behavior:

The user should be routed to the indexController when visiting /.

Actual Behavior:

The user receives the error below because this is reassigned:

[1] node_modules/koa-router/lib/router.js:202
[1]     this.register(path, [method], middleware, {
[1]          ^
[1]
[1] TypeError: this.register is not a function

The answer here is probably to use explicit references to the Router object.

[osdiab]

I have run into this too.

[jbielick]

You need to provide the this context if you're calling object methods with a string (router[route.method]). This guide explains the behavior here. This is how JavaScript works, it is not a bug in this library.

Try the following, which uses call to force a this context.

router[route.method].call(router, route.path, route.action);

[osdiab]

Everybody in this thread understands how this is bound. It’s a fault of the library that calling it in a certain way makes it hard crash, in my opinion; if that’s truly the intention of maintainers they ought to leave a note explaining that, but honestly that’s a cop out IMO.

It can easily be written in such a way to not crash. I may put forth a PR but it’s not high pri enough for me to expedite that.

[jbielick]

The answer here is probably to use explicit references to the Router object.

This doesn't make sense. Are you suggesting calling Router.register? That's not a method. this is an instance of the Router class. register is not a class method.

Everybody in this thread understands how this is bound.

It's not bound; that's your issue.

[osdiab]

Indeed @jbielick, and I bound it as a workaround in my team’s codebase.

But it’s exactly this kind of unexpected crash and lame workaround that makes other people in my team feel that JS/TS sucks and we should be using a “real” programming language. I am convinced otherwise but libraries not having sound APIs makes it harder for me to make that case to teammates who are not as invested as me in the particular tech.

I think expecting the user to “just know” that they need to call functions in the one supported way, and then expecting them to read the source code to figure out what’s happening when it breaks, is counterintuitive. I think that makes it harder to build on top of other people’s code, when using it in different ways causes breakage, as it did for me.

I expect that if the library depends on this it ought to ensure this is bound properly. That can be done by explicitly binding the router object in the library code to the router instance WITHIN THE LIBRARY (if that’s possible, I haven’t read the library code), or exposing some way for the library to internally call register without depending on this (if that’s reasonable, I haven’t read the library code). In ES6 you would probably just use an arrow function instead of a normal one inside the Router class (don’t nitpick if the code isn’t structured that way, I haven’t read all the library code, it’s just a suggestion if you didn’t realize yet).

Perhaps a reasonable maintainer can chime in on what’s possible. Is that inconceivable?

I’m interested in libraries that are robust and composable; while that may not be what you care about, “it’s not a bug, RTFM” and “even though I understand what you mean, the literal exact meaning of your short comment on the web is a tiny bit off, so I’m going to argue that you’re wrong and I’m right” doesn’t really satisfy me.

Sorry for ranting. It’s really not that important an issue. Just these developer troll responses just tick me off sometimes.

[jbielick]

Characterizing my response as "trolling" is a gross misrepresentation. I didn't spend time suggesting a solution, finding a javascript guide that carefully explains the situation and issue, and write a response on this thread because I wanted to to demean you. I'm sorry if you interpreted it that way.

I've attempted to help dozens of people in the issues for this library on my own time for no real gain on my part. I've taught JavaScript classes on several occasions and believe the best way to master your work is to understand what's going on—even if it's tricky. I don't buy into the idea that others should fix it for me.

If I see a couple of people struggling with something that thousands of other users haven't mentioned, I'm going to step in and propose a solution or workaround if it means helping you get unblocked. If contributing a fix for this is beneath you or not worth your time, you're going to be really disappointed at how discouraging and off-putting that attitude is to anyone actively contributing to this library. Moreover, it seems as though nobody is actively contributing to this library, so a workaround is probably your best bet!

So help me out, Omar. Here's a small reproduction of the OP's example:

const Router = require('koa-router');

const router = new Router();

const AppRoutes = [
    {
        path: '/',
        method: 'get',
        action: (ctx) => {
          ctx.body = 'ok';
        },
    }
]

AppRoutes.forEach(route => {
  router[route.method](route.path, route.action);
});

const Koa = require('koa');
const app = new Koa();

app.use(router.routes());

app.listen(3000);

{
  "name": "router-test",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "koa": "^2.7.0",
    "koa-router": "^7.4.0"
  }
}

› curl localhost:3000
ok%  

Maybe I'm missing something—I get no errors. What changes should be made to this library? Assume no ill intent here.

[amaschas]

Hey guys, didn't mean to start a fight, just noticed that this gets reassigned in some cases and this is such a popular package that it's probably an issue for a lot of people. I think the issue might lie with the prototype/newable pattern that this plugin uses, and could be resolved by using a factory pattern instead? I'd be happy to help work up an approach to ensure that the interfaces to this module work the way the user expects in a greater variety of cases. Here's a simple example of a module pattern that avoid this:

function RouterFactor () {

	let testString = "works";

	let get = function() {
  	console.log("this " + testString);
  }
	  
  
  return {
  	get: get
  };
}

let router = RouterFactor();

router.get();

[osdiab]

@jbielick thanks for clarifying your intentions. I was mostly just put off by the statement that it's not a bug, the user is just using it wrong, and then more posts just explaining how we're using it wrong, when i was specifically saying that it should be considered a bug.

I am well aware of how expecting other people to fix issues on demand is annoying in an open-source library, and i'm grateful that you are putting time into helping people use it while no maintainers do. I just also think that claiming that issues people find aren't bugs, because you assume they don't understand how javascript works and they're just "holding the phone wrong," so to speak, is also an annoying attitude.

and my intention is not to demand people to fix the problem for me immediately, which I agree is annoying. because there's already a simple workaround that my developers don't need to encounter frequently, this bug, while real, is just not high enough priority an issue for my team to put dev time into it. as such i want this issue to remain open and remain relevant for somebody to fix at some point, because i think it's a real issue. just, that person won't be me right now since i have other things to focus on, as much as i'd love to spend a lot of time maintaining every open-source library i'm a user of.

thanks for the suggestion @amaschas . i'll see if i can figure out based on that what call in my codebase might lead to the issue, and see if there's a way to rework @jbielick 's code sample to make the problem show up.

[jbielick]

@osdiab Please report back with an example from your codebase that breaks and I would be happy to take a look—though, I don't have publish rights to this package anymore so I can't assist there.