search

JohnnyMorganz / luau-lsp

Language Server Implementation for Luau
MIT License
247 stars 60 forks source link

Make auto require more data model aware? #625

Open LordOfSpelunky opened 6 months ago

LordOfSpelunky commented 6 months ago

I'm experimenting with using Luau LSP internally at Roblox. Auto requires are fantastic, but we have a common pattern of the following:

local Plugin = script:FindFirstAncestor("PluginName")

local Script = require(Plugin.Src.Script)

Unfortunately, it looks like Luau LSP will auto-require without the Plugin part, and will just script.Parent etc, which we don't want.

I think that Luau LSP should (optionally?) use variables if they exist and statically point to the strict data model instance. That is, if we have some require already such that Luau LSP already knows where to put stuff, and it sees that the equivalent to script.Parent.Parent exists in the form of Plugin, it should use that.

I think another problem with trying to use auto requires exclusively for us is that I'm not sure Luau LSP will actually split itself off from the Plugin variable, but instead might try to put itself at the beginning. Would be cool to also be able to tell it to place the first require wherever it can find a strict data model instance?

JohnnyMorganz commented 6 months ago

Sounds reasonable to support to me! We can definitely be more clever in our auto require logic, the current implementation is quite rudimentary.

I'm not sure Luau LSP will actually split itself off from the Plugin variable, but instead might try to put itself at the beginning.

I think this should follow automatically by implementing the first point - we should add the require after the fixed variable.

LordOfSpelunky commented 6 months ago

Cool 😃

As a side note, I forgot to mention that ideally it puts some whitespace around itself too like the GetService option

JohnnyMorganz commented 6 months ago

Do you mean whitespace between the fixed variable (Plugin) and the block of requires (Script etc.?) Also do you reckon this should follow the existing config completion.imports.separateGroupsWithLine? Or whitespace between the variables should be a separate config

LordOfSpelunky commented 6 months ago

Yeah basically I mean that I want to make sure that in the following code:

local Plugin = script:FindFirstAncestor("PluginName")

local function doinStuff()

...that the first require gets placed like:

local Plugin = script:FindFirstAncestor("PluginName")

local Library = require(Plugin.Src.Library)

local function doinStuff()

No idea WRT separateGroupsWithLine. I think it makes sense to put it under there on first blush though.