luvit / lit

Toolkit for developing, sharing, and running luvit/lua programs and libraries.
http://lit.luvit.io/
Apache License 2.0
245 stars 58 forks source link

Some luvit-loader fixes/improvements #191

Closed squeek502 closed 8 years ago

squeek502 commented 8 years ago

Fixes for some some issues I ran into while making luver.

Let me know if you'd like any more information about any of the changes or if you see a better way to handle any of these issues.

squeek502 commented 8 years ago

Another thing I just realized: require automatically adds a key to package.loaded using the module name that is passed in, which can cause some weirdness. For example:

-- /usr/main.lua
local r = require("./relative.lua")
-- luvit-loader will set package.loaded['/usr/relative.lua'] and 
-- require will set package.loaded['./relative.lua'] to the return value of the loaded file
assert(package.loaded['./relative.lua'] == package.loaded['/usr/relative.lua'])

require("sub.test")
-- /usr/sub/test.lua
local r = require("./relative.lua")
-- r will be the cached value of package.loaded['./relative.lua'], 
-- not the return value of loading /usr/sub/relative.lua
assert(package.loaded['./relative.lua'] == package.loaded['/usr/relative.lua'])
assert(package.loaded['/usr/sub/relative.lua'] == nil)
creationix commented 8 years ago

Yes, the package.loaded insertion weirdness is why I insert this loader at the front of the list, even before the one that looks in package.loaded.

squeek502 commented 8 years ago

As far as I can tell, that lookup is done in require itself, not in a loader, see https://github.com/lua/lua/blob/5.1.x/src/loadlib.c#L454-L461

creationix commented 8 years ago

That's what I thought as well, but when I was testing it, I did find that by inserting my loader first, it somehow got around the problem. It's been a while so I may be remembering something wrong.

creationix commented 8 years ago

Did you still want to merge this in. Sorry I got distracted with travels.

squeek502 commented 8 years ago

Yeah, I think this is good to go. The '@' prefix when loading bundled dependencies would require a larger rewrite and potentially require changes to luvi (since it also uses the 'bundle:' prefix).