lapis icon indicating copy to clipboard operation
lapis copied to clipboard

Lua app.include is in a weird place

Open karai17 opened this issue 6 years ago • 7 comments

So I was rewatching the 2016 talk that Leaf did on Lapis and I really liked the sub-application stuff. I tried restructuring my application to use sub-applications and I ran into an error when using the below syntax:

image

error: ./app.lua:4: attempt to call method 'include' (a nil value)

I did a bit of investigating and found issue #255 that asks for the sub-application syntax to be added to Lua, not just Moonscript. This issue is from 2015 and the aforementioned talk was from 2016 so I figured it was added. I did a bit of digging through the Lapis lua code and I found this:

https://github.com/leafo/lapis/blob/master/lapis/application.lua#L287

It looks like include is being added to __class instead of __index and thus is not able to be called normally. __init, __base, __name, find_action, and before_filter are also located there, though before_filter is duplicated in __index too. the application itself only holds a singler property, router.

When I try to call app.__class:include("apps.admin"), I get the following error:

error: /usr/share/lua/5.1/lapis/application.lua:296: bad argument #1 to 'pairs' (table expected, got nil)

https://github.com/leafo/lapis/blob/master/lapis/application.lua#L296 shows that other_app doesn't have a __base property which is quite true, that is located in __class as mentioned above.

So I'm not quite sure what to do here. I just spent a fair bit of time restructuring my code and I don't exactly want to undo all of that work. This is clearly a bug, but I'm not sure exactly what the bug is. Is Moonscript generating unexpectedly? From what I can tell with very minimal understanding of Moonscript, the function definitions prefixed with @ seem to be going into __class whereas the rest are going into __index. Is this intended behaviour? Is there a simple fix for this bug that could see a quick patch release of Lapis?

karai17 avatar Jun 02 '18 14:06 karai17

After a bit more tinkering, I got the following to work:

in the root application, I have the following code:

local lapis = require "lapis"
local app   = lapis.Application()

app.include = app.__class.include
app:include("apps.core", nil, app)

return app

In the sub-applications, I have the following code:

local lapis = require "lapis"
local r2    = require("lapis.application").respond_to
local app   = lapis.Application()

app.__base = app
app.name   = "core."

app:match("home", "/", r2(require "apps.core.home"))

return app

Clearly not the best solution, but it seems to function (though I am scared to know what the internal data looks like and how this will bite me in the rear).

karai17 avatar Jun 02 '18 14:06 karai17

mmm, this was something I put into the talk thinking it would be ready by then, but I never had a chance to get it finalized. So it's not a bug, but a lost feature :(

The class-level include method doesn't work on Lua based apps because of how they are instantiated:

  • In MoonScript you create a subclass
  • In Lua you instantiate the class, then add configuration data to the instance

This approach hasn't been ideal. Many of the class methods are copied over to be instance methods like you did in your fix above.

I want to make the Lua version work by creating subclasses, but it means changing the function you call to create an app, which would be a backwards incompatible change. After this change though, everything should work the same across both languages.

leafo avatar Jun 05 '18 23:06 leafo

Any timeline on when those changes might be made?

Edit: FYI, I think a Lapis 2.0 release which brings MoonScript and Lua versions into complete feature parity would be a perfectly acceptable release, even if that's the only major change. Tagging it as 2.0 would also make the incompatible API more palitable. :)

karai17 avatar Jun 06 '18 00:06 karai17

I think that 2.0 should hopefully also include full 5.2 and 5.3 compatibility if possible.

RyanSquared avatar Jun 08 '18 15:06 RyanSquared

I think this change makes sense for 2.0. Any other breaking changes on your wishlist?

leafo avatar Jun 08 '18 21:06 leafo

Perhaps we should set up a v2.0 milestone that we can attach issues to so we can build a feature list/wishlist.

  • Feature parity between Lua and MoonScript
  • Lua 5.2 and 5.3 support
  • Finish MySQL driver (I only bring this up because the issue tracking mysql support is 6/7 complete, not usre if this is out of date or just being deferred)
  • Create SQLite driver

I think those are the major features I can think of, only the first (two?) being breaking changes.

karai17 avatar Jun 08 '18 21:06 karai17

Minor update on this. I noticed that sub-apps that have path prefixes require a path name, even if there is no name prefix set. Is this expected behaviour, or is this a quirk with my Lua hacks?

karai17 avatar Oct 10 '18 03:10 karai17

Closing outdated issue. The original issue was addressed in https://github.com/leafo/lapis/releases/tag/v1.11.0

leafo avatar Feb 10 '23 22:02 leafo