espruino / EspruinoTools

JavaScript library of tools for Espruino - used for the Web IDE, CLI, etc.
Apache License 2.0
150 stars 89 forks source link

Improvements to localModules #169

Open mariusGundersen opened 11 months ago

mariusGundersen commented 11 months ago

Background: I'm working on a VSCode extension for Espruino.

The localModules file assumes that the source code is in the process.cwd(), which isn't always true. For my scenario the cwd is completely different from the file location, since the vscode window runs somewhere else entirely. It would be nice to be able to set the value of cwd somehow in the module.

There is also a problem with the loadEspModules function, which uses (essentially) fs.readFileSync(path.join('modules', moduleName)), which ends up with an implicit process.cwd().

I also don't think it behaves like require() should do, since it always loads it relative to the current process, not the file that requires it. For example, given that main.js: requires('./folder/file.js') and folder/file.js: requires('./anotherFile.js), then it should find folder/anotherFile.js, but since it doesn't use the current location of the file that required it, it ends up looking in the root folder. For this to work we need to know the path of the current module when it looks for the next module. It would then have to rewrite the code so that the url is absolute, for running on the device. This is a bit more complex to solve, and would have to involve the modules.js file too. The above changes can be done in localModules.js only.

gfwilliams commented 11 months ago

Ok, thanks! Yes, that sounds like it's definitely something that needs changing - looking relative to the current file's directory seems the way to go.

While it might be nice to send the current file path in data for Espruino.addProcessor("getModule", function (data, callback) {, that itself is a bit tricky since when running in the browser we don't know the file path.

I guess we could do that and then at the very least we can pass the directory through when requiring one module from another module?

Up to you, but yes, as long as it doesn't break the IDE I'm all for getting this working nicely

mariusGundersen commented 11 months ago

"getModule" is initially called from "transformForEspruino" which only had the code, not any other parameters, so it wouldn't be possible without breaking changes to pass in the cwd there. Instead there could be a Espruino.Plugins.LocalModules.SetCwd(cwd) method for overriding the process.cwd() default. This would have to be called before "transformForEspruino".

To make relative paths work there needs to be a lot of changes in Espruino.Core.Modules, which is why I started refactoring it. I'm not sure it's worth it, it seems to me like a better idea is to put all modules in the modules folder (and maybe have a config for it being called espruino_modules, as mentioned in the code comments).