Set state by default?

[bitinn]

Now that I think of it, we should just generated random state by default, right? This is the secure approach and grant is already using session (which it might as well use to store state).

Any reason for us to do a dynamic overwrite manually?

[simov]

The state parameter is optional for most of the providers, but I agree that the dynamic override approach is required to set the state, as setting up the state in the json configuration doesn't make sense. I'll think about adding this feature as an option, thanks for suggesting it 👍

[simov]

@bitinn I just pushed the auto generated state here e7d25e4

If you set the state through the json configuration or through dynamic override, that state probably would be either string or number type. In these cases that will be the state used, any other falsy value means no state.

However if you set the state to true in your server configuration for example (because that's global for all providers) then a six digit random number state will be generated for you on each authorization attempt.

[bitinn]

@simov just a note to make sure you are aware of koa and koa-bodyparser versions, both just got bumped again.

I am not sure what's the best approach here, but given how fast koa and its core middlewares evolves, a strict version dependency on them is probably not what you want.

I believe we can't use npm shrinkwrap to workaround this neither, because it only works if the my app require a lower version of koa than grant-koa, while currently the opposite is true, which results invalid package error.

This is generally a problem when mounting a full koa app into existing koa app, unfortunately.

[simov]

@bitinn is there a real problem with that? Take a look at this example https://github.com/simov/grant/blob/master/example/koa/package.json currently the consumer app is using 0.14 and grant-koa is using 0.18.1 same for the body-parser middleware - the versions are different. Still the app seems to be working, but that's a really simple example, that's why I'm asking about any potential problems with that.

[bitinn]

@simov probably not, other than unmet dependency warning during npm install

say you have a koa app using latest koa and bodyparser:

npm WARN unmet dependency /node_modules/grant-koa requires koa@'0.18.1' but will load
npm WARN unmet dependency /node_modules/koa,
npm WARN unmet dependency which is version 0.19.0
npm WARN unmet dependency /node_modules/grant-koa requires koa-bodyparser@'1.4.1' but will load
npm WARN unmet dependency /node_modules/koa-bodyparser,
npm WARN unmet dependency which is version 1.5.0

[simov]

Interestingly enough I'm not getting that warning with node 0.12.2 and the above example.

Other than that the solution would be to use peerDependencies like this

"peerDependencies": {
  "koa": "0.x",
  "koa-bodyparser": "1.x"
}

or I can just leave the version number a bit more relaxed.

I'm really not sure how Koa implements the mounting, but with Express I can have express 3.x app that mounts grant with express 4.x app and middlewares in it without a problem.

[bitinn]

npm install won't give unmet dependency error if it can find a version meeting both grant-koa and my koa app's requirement.

But my app set koa dependency as koa: 0.19.0, a strict version, causing npm failing to find a release that satisfy both my app and grant-koa.

You can imagine some developers doing this before they ship to production server, or when they specifically try to avoid a buggy version.

To me there really isn't much one can do except: update dependency, or relax dependency. On my end the best alternative would be to check-in node_modules and avoid running npm install for production.

[simov]

Ok, I'll figure out something, but still I'm not sure how you get that warning. Can you experiment with this example https://github.com/simov/grant/tree/master/example/koa

As you can see currently there are older versions set than the ones used in grant-koa (that's the first test) Then I removed node_modules all together and set the latest versions in the package.json for that app. The result was exactly the same - no warnings whatsoever.

[bitinn]

@simov Thx for looking into it, I will try and report back in the weekend, it's now well after midnight on my side :)

[bitinn]

Ah I may have identified the problem, it would appear that when upgrading or installing grant-koa 3.0.3, npm only installed koa-route, grant and thunkify packages, leaving dependency of koa and koa-bodyparser to my app's node_modules, thus the unmet dependency errors.

npm uninstall grant-koa
npm install

Appear to fix this problem, I am not sure about the exact steps to reproduce it, but at least I now know it's not grant-koa package.json's problem.

TL;DR, your example works, my node_modules have problems.

[simov]

That makes sense, I've seen this warning before.

As I mentioned a few comments back, there are two available options, but I'm still wondering which one would be better

"peerDependencies": {
  "koa": "0.x",
  "koa-bodyparser": "1.x"
}

"dependencies": {
  "koa": "0.x",
  "koa-bodyparser": "1.x"
}

[bitinn]

@simov If you don't use any specific features of koa, other than some general routing, then a relaxed peerDependencies might just be good for grant-koa.

But, if you want better control of koa and koa-route dependency, then you should use dependencies, because unlike other koa middleware, your grant-koa is a koa app itself, so you actually make use of koa, unlike in peerDependencies general usage, where plugins/middlewares don't depend on their host, but want to make sure host is a certain supported version. (most koa middleware doesn't have koa as a dependency).

It's a close call, for flexibility I might prefer peerDependencies, but your current design would call for dependencies.

[simov]

Version 3.1.0 is published on NPM changelog

For now I'm using relaxed dependencies for the koa-middleware, like this

"dependencies": {
    "thunkify"        : "2.1.2",
    "koa"             : "0.x.x",
    "koa-route"       : "2.4.0",
    "koa-bodyparser"  : "1.x.x",

    "grant"           : "3.1.0"
  }