Adding plugin support

[JohnAD]

Please do not accept this pull request. It is a work in progress [WIP] and I'm only placing this here for possible feedback as I build this.

TODO:

• Test against various router scenarios
• Add plugin support for optional modification of result response tuple.
• Test threaded usage
• Write at least one (but ideally two) plugins that are ready to publish in nimble.directory
• Write unit test
• Write documentation:
  • RST instruction file linked to by README
  • Write a "how to write a plugin" RST file
• remove all those debugging echo statements; clean up code

The first plugin I write will be for using cookies for cross-page messaging. It would be used something like:

import jester
import jestercookiemsgs

routes:
  plugin cmsg <- cookieMsgs()
  get "/":
    resp indexPage(request, cmsg)
  post "/":
    if request.params["name"] == "Joe":
      cmsg.say("success", "I like that name.")
      redirect "/hello"
    else:
      cmsg.say("danger", "That isn't a name I like.")
      redirect "/"
  get "/hello":
    resp helloPage(request, cmsg)

[JohnAD]

Looks like I'll be adding a third handle to the plugin, for the automatic redirect or response based on conditions. That way, an auth plugin, for example, can do something like redirect traffic if a particular router always requires an authenticated user.

Philosophically, I'm not making it easy to write a plugin; the concentration is on ease of use of plugins by the website developer. The plugin library author, on the other hand, will be jumping through quite a few hoops.

[JohnAD]

Hmmm...other routers referenced by "extend" end up globbing together into a single "allRoutes" block. I'm going to be changing that so that plugins and other things are decoratable on a per-router basis. I'll be explicit here in how it changes things. It should have no effect on performance.

Edit: actually I'm keeping the "globbing together" behavior. Changing that would likely break a lot of existing websites. Instead I've added a specific command to routers to allow modifications on a per-router prefix basis.

[JohnAD]

Ready for review... mostly. Two quick items:

1. I cannot seem to get nimble test to work on my local machine. (I'm running PopOS, an ubuntu derivative, and using Nim 1.0.4). I keep getting an odd compiler error. Travis CI (via GitHub) seems to be happy with the test suite, so I've used it for getting the final bugs out. Not really an ideal way to do things. So, I've written what could be a test but have commented it out since I can't actually run it yet.

   I'll keep workin on my machine to get testing going.

2. A logistic question: Right now, any extra user parameters passed to the plugin are only passed to the [x]_before procedure. I can't think of a reason that the [x]_after would need to see them again. But I could be missing something a future plugin might need. Do you I think I should change the convention to pass them (and their current values) into [x]_after procedure also?

Also, I've got two plugins working. It is probably best to try it out with the cookieMsgs plugin since the other would require access to a mongodb server. I've also got three other private plugins working (simultaneously) on one of my web sites. Links to the public plugins:

• https://github.com/JohnAD/jestercookiemsgs
• https://github.com/JohnAD/jestermongopool

I've also written two doc files and have them linked from the README.

[JohnAD]

If curious, the error I'm getting on nimble test is:

Hint: asyncpty [Processing]
Hint: asyncsync [Processing]
/home/johnd/.nimble/pkgs/asynctools-#pr_fix_compilation/asynctools/asyncsync.nim(159, 15) Error: type mismatch: got <proc (){.closure.}>
but expected one of: 
proc callSoon(cbproc: proc () {.gcsafe.})
  first type mismatch at position: 1
  required type for cbproc: proc (){.closure, gcsafe.}
  but expression 'wakeupAll' is of type: proc (){.closure.}
  This expression is not GC-safe. Annotate the proc with {.gcsafe.} to get extended error information.

expression: callSoon(wakeupAll)
       Tip: 6 messages have been suppressed, use --verbose to show them.
     Error: Execution failed with exit code 1
        ... Command: "/home/johnd/.nimble/bin/nim" c --noNimblePath -d:NimblePkgVersion=0.4.3 --path:"/home/johnd/.nimble/pkgs/httpbeast-0.2.2" --path:"/home/johnd/.nimble/pkgs/asynctools-#pr_fix_compilation" --path:"/home/johnd/.nimble/pkgs/asynctools-#pr_fix_compilation" "-r"  "tests/tester"
stack trace: (most recent call last)
/tmp/nimblecache/nimscriptapi.nim(165, 16)
/home/johnd/Projects/jester/jester_5800.nims(27, 8) testTask
/home/johnd/.choosenim/toolchains/nim-1.0.4/lib/system/nimscript.nim(252, 7) exec
/home/johnd/.choosenim/toolchains/nim-1.0.4/lib/system/nimscript.nim(252, 7) Error: unhandled exception: FAILED: nimble c -y -r tests/tester [OSError]
     Error: Exception raised during nimble script execution

I wonder if I need a specific version of the compiler...

[JohnAD]

Yep. Downgraded nim to 0.19.4 to get nimble test to work.

So, I have tested the test code; fixed it, and..as a bonus... fixed a newly discovered edge case bug. :)

This will be the last commit I make so that you can examine unchanging code.

[JohnAD]

In creating the https://github.com/JohnAD/jesterjson plugin, I discovered a fairly serious bug: since the "wildcard" parameters of a URL are not put into request.params until after the route block has started, those entry(s) are missing. I can totally see where that would cause a problem.

And, moving where the [pluginName]_before() proc is called is not an option because of scope issues. Thus, the only answer is to create another procedure call based on naming convention and have it launch after the wildcard matching is done.

While I'm at it, I'm going to:

1. Drop the _before bit for the start of the plugin. That will make nimble doc work intuitively.
2. Use if declared before the _route and _after calls to make them optional rather than required.

So, instead of:

• [pluginName]_before
• [pluginName]_after

It will be:

• [pluginName] (formerly [pluginName]_before)
• [pluginName]_route
• [pluginName]_after

I hope to have this done and documented in the next 24 hours. But I am, as always, willing to change direction if anyone has feedback.

Thoughts?

[JohnAD]

Wow. That was easier to update than I thought it would be. All tests pass; the docs have been updated.

Ready for review once again!

[JohnAD]

@dom96 I'm at FOSDEM. If interested in meeting up on this (or anything else), email me at [redacted-from-public].

[dom96]

Hey @JohnAD, sorry I missed your messages. It was nice meeting you at FOSDEM, it's a shame we didn't get a chance to speak for longer!

Please feel free to email me to ping me about this PR, my email is at https://picheta.me. I currently don't have the time to review this PR as it's quite large, so it may take me a while to get through it. It looks like you've implemented some really cool functionality though!

[skellock]

@JohnAD thx for the PR! I had a chance to kick some tires and it seems to be working well.

I did have a few head scratches but I think that was due to me really not being very good at nim (also i have preconceptions on how it should work because of my background with js and rails).

For example, I wanted to have no return value from a plugin, but seemed to be forced to use one.

Another hiccup I ran into was not having the correct modules in scope at the top-level. A well-placed export seemed to do the trick (and my brain hurts trying to figure out why). I believe that's working as intended in nim though.

I'd say all-in-all this was a success.

Perhaps I've spent too much time in rails and js land, but having a 800kb standalone binary running in single threaded mode hitting 5000+ req/sec (including redis calls!) and using under 9 MB of memory under load kinda blows my mind. To pieces.

Thank you both @JohnAD and @dom96.

Edit 1: Typos. Typos everywhere.

[JohnAD]

@skellock, Thanks for testing out the PR to such an extent!

I could certainly could add a syntax variant that did not create a variable. So that:

routes:
    plugin newJsonPostsOnlyPlugin()
    plugin sample <- newSamplePlugin("Steve")

would both work.

I've also toyed with the idea of making it look like an actual assignment statement. In which case it could look like:

routes:
    plugin discard newJsonPostsOnlyPlugin()
    plugin var sample = newSamplePlugin("Steve")

The problem with that direction is that it implies that the text after plugin is just generic nim code. But it isn't; it is a very specific syntax.

[JohnAD]

As discussed in more detail here:

#226 (comment)

I will be creating a temporary fork available on nimble and the merger of plugins will occur at a later date. Meanwhile, I will go ahead and close this PR.