Add import resolution plugin hook by SamuelRobertBond · Pull Request #1455 · TypeScriptToLua/TypeScriptToLua

SamuelRobertBond · 2023-06-06T02:48:58Z

I was having issues with files with a . in the file name (e.g physics-api.component.ts) and needed some method of providing custom filepath resolution login when importing files from from a library into another project bundling the library.

SamuelRobertBond · 2023-06-06T22:12:46Z

I found some issues with paths in this, currently working through it. Let me know if you have any thoughts on what I can add or change. The particular issue was with using tstl.transpileProject in another project to compile the bundle implementing the libs. ~~The fileDirectory come through with subpaths, compared to when using the tstl command, which give the /dist directory of the lib.~~ The problem occurs when a dependency imports another file in the lib.

from other scripts

…ptToLua

SamuelRobertBond · 2023-06-07T00:19:30Z

Fixed the issue and moved the plugin hook call to resolveLuaDependencyPathFromNodeModules. I uploaded an example usage here

SamuelRobertBond · 2023-06-07T22:33:31Z

Another idea I had for changes to the plugins would be to allow plugins definitions to be passed through the compiler options when using tstl through the api. It would be a separate issue, but I wanted to get your thoughts on the current changes before I start messing around with that.

This is an example of what im talking about, although I may just not be understanding the TransformerImport

the getPlugins call again

Perryvw · 2023-06-08T20:02:08Z

        result: EmitFile[]
    ) => ts.Diagnostic[] | void;
+
+    onImportResolutionFailure?: (packageRoot : string, dependency : string) => string | undefined


A module resolution plugin is something we have long been considering, and I think a better interface would be something like:

interface ModuleResolutionRequest { moduleIdentifier: string; // "foo.bar.baz" from require("foo.bar.baz") requiringFile: string; // path of the (lua) file containing the require // maybe some other stuff I'm forgetting now? // maybe a 'defaultResolve: (ModuleResolutionRequest) => string' to call the default behavior? // Not sure why anyone would want that } moduleResolution?: (moduleToBeResolved: ModuleResolutionRequest) => string | undefined

Then crucially this plugin would be executed before our regular module resolution process, allowing users to intercept before it even starts. If it returns undefined it will go into the default behavior. I'm just wondering if we want to somehow maybe also be able to indicate that a specific import should not be emitted, but maybe that's for a future version.

I think this interface makes more sense, it gives more control to someone writing a plugin to identify the package they wish to use via the module identifier. Ill start working on it.

Perryvw · 2023-06-08T20:03:04Z

        result: EmitFile[]
    ) => ts.Diagnostic[] | void;
+
+    onImportResolutionFailure?: (packageRoot : string, dependency : string) => string | undefined


I don't like you passing packageRoot to the outside world, that's really something internal to the tstl implementation. What is the reason you need this in your case? I'm trying to understand the usage scenario

In my particular case, I was getting the path that was being searched for the package and the attempted path to the dependency and transforming that to what I needed it to be, so when the plugin. Before I made the changes to packageRoot, the path being passed to the plugin was inconsistent. This was just much more consistent for me to work with. I think the changes to the interface and location for the plugins call will resolve this issue.

Perryvw · 2023-06-08T20:07:56Z

+            }
+        }
+
+        if (this.plugins != null) {


I don't think this should be exclusive to nodeModules paths, I think a nicer place for this would be in resolveImport, before the call to resolveDependencyPath (going with the idea I mentioned on the plugin interface above, where users can intercept these calls before we do all of this quite complicated work on the file system while we already know we won't find anything)

Perryvw · 2023-06-08T20:15:51Z

            path.join(resolvedPath, "init.lua"), // lua looks for <require>/init.lua if it cannot find <require>.lua
        ];
+
+        if (resolvedPath.endsWith(".lua")) {


I don't think this if statement should be here at all. If the resolved path ends with .lua we do not have to search for it, because we apparently we already have a lua file path. I don't think this is possible in regular tstl module resolution, but I guess your plugin triggers this? I don't think this code path should really be possible ever.

I put this here as a convince thing for me. When I was constructing the path it I included .lua in the resolved path. This was not being found since the possible files above appends .lua to the resolve path, the path was being set to /my/path/file.lua.lua. I didn't want to just add resolvedPath itself to the possible files, because someone could intentionally provide a path to a non lua file. I think the changes you mentioned above would make this a non issue though.

changes to the interface

no longer needed

SamuelRobertBond · 2023-06-10T00:38:34Z

I made some of the changes requested. I didnt include defaultResolve yet. I also still need to write the test. Figured I'd get them out there in case you wanted to take a look. Ill probably write the test tomorrow.

Perryvw

Starting to look good, need a test still, and of course you should make sure this works for your use-case too still. Also to avoid our formatter complaining, you can run npm run lint and npm run fix:prettier locally to make sure there are no formatting or linting complaints.

Perryvw · 2023-06-10T12:52:53Z

    }

+    private resolveDependencyPathsWithPlugins(required: ProcessedFile, dependency: string) {
+        if (this.plugins != null) {


This check is not required (and is missing the case where it is undefined instead of null). Tstl is using strictest TS rules, it will not allow you to pass null to this (and will prevent you from trying to use something that could be).

You can just remove it

Perryvw · 2023-06-10T12:55:05Z

+            const dependencyPath = requiredFromLuaFile ? luaRequireToPath(dependency) : dependency;
+
+            for (const p of this.plugins) {
+                try {


I'd just get rid of this try/catch. The plugin shouldn't throw, and if it does I'm fine with tstl just aborting

Perryvw · 2023-06-10T12:58:51Z

        result: EmitFile[]
    ) => ts.Diagnostic[] | void;
+
+    moduleResolution?: (moduleIdentifier : string, requiringFile : string) => string | undefined


you probably also want to pass the options and emitHost to this plugin, like the functions above it have

interface - removed try catch on plugin execution

- added test case

SamuelRobertBond · 2023-06-12T03:52:57Z

Fixed an issue I found with the path resolution and added the test specified. I ran and fixed the formatting with the linter and ran the entire test suite with everything passing. I wasn't sure if I should add a test in the plugins spec as well. I included the moduleResolution plugin for the test in the plugins folder, but I wasn't sure if that was the appropriate place for it. Let me know if there's anything else I should include in the test or anything that needs to be changed.

Perryvw · 2023-06-12T19:38:47Z

+            if (plugin.moduleResolution != null) {
+                const pluginResolvedPath = plugin.moduleResolution(
+                    dependency,
+                    dependencyPath,


this parameter is the requiringFile, shouldn't this be required.fileName?

I missed that, which make sense why I ran into the issues I did, I'll fix that ty

Perryvw · 2023-06-12T19:40:52Z

+                    const isRelative = ["/", "./", "../"].some(p => pluginResolvedPath.startsWith(p));
+
+                    // // If the import is relative, always resolve it relative to the requiring file
+                    // // If the import is not relative, resolve it relative to options.baseUrl if it is set
+                    const fileDirectory = path.dirname(required.fileName);
+                    const relativeTo = isRelative ? fileDirectory : this.options.baseUrl ?? fileDirectory;
+
+                    // // Check if file is a file in the project
+                    const resolvedPath = path.join(relativeTo, pluginResolvedPath);
+                    const fileFromPath = this.getFileFromPath(resolvedPath);


This looks just straight up duplicate from logic in resolveDependencyPath, please move to a shared function they both can use

Perryvw · 2023-06-12T19:42:07Z

+const plugin: tstl.Plugin = {
+    moduleResolution(moduleIdentifier, requiringFile) {
+        if (moduleIdentifier.includes("foo")) {
+            return requiringFile.replace("foo", "bar");


the requiring file is called main.ts (or .lua?), not foo, so I'm confused by this plugin. See also my other comment regarding the requirintFile parameter

the test plugin

the format specified in the interface

SamuelRobertBond · 2023-06-12T23:07:20Z

Sorry about the silly mistakes, this should make more sense now. I removed const moduleIdentifier = requiredFromLuaFile ? luaRequireToPath(dependency) : dependency. and changed it to pass in the identifier as the format specified in the interface above. Thank you again for taking time to look at this, let me know if there's any other changes I need to make.

Perryvw

Looks good to me. Thanks for your contribution!

SamuelRobertBond and others added 2 commits June 5, 2023 22:31

added a plugin hook for custom module resolution strategies

8ca8373

Merge branch 'TypeScriptToLua:master' into master

14ca7a7

SamuelRobertBond mentioned this pull request Jun 6, 2023

Dots in module path breaks Lua import in a non-bundle compilation #1445

Closed

SamuelRobertBond added 3 commits June 6, 2023 19:43

fixed an issue with the plugin module resolution when importing scripts

7d77a1e

from other scripts

Merge branch 'master' of https://github.com/SamuelRobertBond/TypeScri…

e9215bc

…ptToLua

fixed a mistake I made when changing packageRoot to currentPackage

f30f500

SamuelRobertBond added 2 commits June 7, 2023 19:54

passed plugins directory to the emitPlan function so we can avoid using

5d098db

the getPlugins call again

delete an accidental output

0bc4fc7

Perryvw reviewed Jun 8, 2023

View reviewed changes

SamuelRobertBond added 2 commits June 9, 2023 20:30

moved module resolution logic to resolveImport and made the requested

a45d888

changes to the interface

reverted resolveLuaDependencyPathFromNodeModules because the changes are

2a73c57

no longer needed

replaced some comments and change a conditional statement

37187f4

Perryvw reviewed Jun 10, 2023

View reviewed changes

SamuelRobertBond added 2 commits June 11, 2023 01:21

- included compiler options and emit host in module resolution plugin

4164d68

interface - removed try catch on plugin execution

- some fixes to the module resolution

1e64c54

- added test case

Perryvw reviewed Jun 12, 2023

View reviewed changes

SamuelRobertBond added 2 commits June 12, 2023 18:28

fixed passing the dependency path instead of the required path and fixed

e3dbf77

the test plugin

removed transformation to luaRequirePath so the module identifier meets

b8cb206

the format specified in the interface

renamed param for formatPathToFile

1a05bfe

Perryvw approved these changes Jun 15, 2023

View reviewed changes

Perryvw merged commit 278ead6 into TypeScriptToLua:master Jun 15, 2023

Uh oh!

Uh oh!

Conversation

SamuelRobertBond commented Jun 6, 2023

Uh oh!

SamuelRobertBond commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SamuelRobertBond commented Jun 7, 2023

Uh oh!

SamuelRobertBond commented Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SamuelRobertBond Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SamuelRobertBond Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SamuelRobertBond commented Jun 10, 2023

Uh oh!

Perryvw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SamuelRobertBond commented Jun 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SamuelRobertBond commented Jun 12, 2023

Uh oh!

Perryvw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SamuelRobertBond commented Jun 6, 2023 •

edited

Loading

SamuelRobertBond commented Jun 7, 2023 •

edited

Loading

SamuelRobertBond Jun 8, 2023 •

edited

Loading

SamuelRobertBond Jun 8, 2023 •

edited

Loading