Absolute import path slashes normalized by hazzard993 · Pull Request #390 · TypeScriptToLua/TypeScriptToLua

hazzard993 · 2019-02-10T04:35:04Z

I've noticed getImportPath will create absolutePathToImport which contains backslashes on Windows.

rootDir seems to always use forward slashes.

So when this happens:

absolutePathToImport.replace(this.options.rootDir, "").slice(1)

rootDir may not be subtracted from aboluteImportPath correctly.

This results in something like:

local __TSTL_Player = require(":.Users.hazzard993.Desktop.test.src.Player");

This PR is just to ensure that absolutePathToImport contains forward slashes to correct the require path when rootDir is used.

Perryvw · 2019-02-10T12:05:22Z

Could you add a couple tests for this? You can probably do something like:

util.transpileString(`import * from "somePath";`, { rootPath: "Test/Root/Path" })

hazzard993 · 2019-02-10T23:53:22Z

Added some require tests.

It got a bit odd trying to figure out how to use rootDir and baseUrl correctly.

In getAbsoluteImportPath, baseUrl causes the transpiler to potentially set up an absolute path to the specified module, this can't work for Lua, can't be tested using the current tools and this option doesn't cause this behavior in tsc. tsc just leaves the require path alone even when baseUrl is specified during transpilation.

The behavior of rootDir in getImportPath I think is redundant as the same relative path is calculated or the program leads into the baseUrl behavior which has the issues above.

From what I understand...

rootDir specifies the directory for all the source files.

baseUrl is used to specify a directory with non-relative/globally available definitions, the transpiler doesn't assist in making them available.

Hope this is ok, just trying to keep the behavior of the options the same.

Perryvw · 2019-02-11T08:29:55Z

Thanks for adding the tests. I'm happy with the PR, the only unknown for me now is the behavior of those config options. To clarify: We aim to have these options work the same in tsc and tstl. I'm not entirely sure on their behavior right now, I'll do some testing later today!

lolleko · 2019-02-11T09:37:37Z

src/LuaTransformer.ts

+            .replace(".\\", "")
+            .replace("./", "")
+            .replace("/", ".")
+            .replace("\\", ".");


Backslashes can be used in filenames in some POSIX environments and should not be replaced on POSIX.

Ideally filePath should be converted using nodes path lib, maybe just using path.sep (https://nodejs.org/api/path.html#path_path_sep) is enough.

Can TypeScript and Lua reference a module on a POSIX system with a backslash in its name?

Not sure, never tried it. Nevertheless it would be nicer to use a platform independent solution here.

I didn't use path.sep because Windows paths can have either one of the slashes. But I have made it so non-win32 systems do not have their backslashes removed.

lolleko · 2019-02-11T09:52:39Z

src/LuaTransformer.ts

    private getImportPath(relativePath: string): string {
-        // Calculate absolute path to import
-        const absolutePathToImport = this.getAbsoluteImportPath(relativePath);
-        if (this.options.rootDir) {


While I agree that the rootDir Implementation is a bit weird and needs a rework, along side with baseUrl(s) you should not just remove it.
This will break paths that include files from a parent directory.

For example 2 files src/lib.ts and src/subdir/subdir2/nested.ts
nested.ts:
import * as lib from "../../lib.ts

The above should get converted to require("lib"). Which I think is what the current implementation does via path.resolve and replace.

Probably one of the oldest pieces of code in this project, which could explain why there are no tests for it.

So please also add tests for paths with ...

EDIT:
FYI rootDir is always set: If not specified via CLI or config rootDir is set to the project or working directory.

Thanks for the info, I'll keep this use case working by adding the code back.

My current tests are just matching the literal string in the require path to what you'd expect in a Lua program.

I don't think I can add add tests for .. since to test the workaround functionally there may need to be a testing tool to execute a Lua project involving multiple files at different locations.

I'll add the code back but I can't figure out how to build some test cases for it.

Figured out a way to test .. paths, modified Compiler.ts to do it.

ark120202 · 2019-02-18T10:48:12Z

src/LuaTransformer.ts

-    private pathToLuaRequirePath(filePath: string): string {
-        return filePath.replace(/\.json$/, '').replace(new RegExp("\\\\|\/", "g"), ".");
+    private formatPathToLuaPath(filePath: string): string {
+        if (process.platform === "win32") {


Lost during merge:

Suggested change

if (process.platform === "win32") {

filePath = filePath.replace(/\.json$/, '');

if (process.platform === "win32") {

Good find, adding that back

TheLartians · 2019-02-18T12:24:54Z

src/LuaTransformer.ts

            .replace(/\.\//g, "")
-            .replace(/\//g, ".");
+            .replace(/\//g, ".")
+            .replace(/\.json$/, "");


Could you do the json replacement fist? Otherwise it would break modules named json, see #418.

ark120202 · 2019-02-18T21:55:45Z

src/LuaTransformer.ts

                .replace(/\\/g, ".");
        }
        return filePath
+            .replace(/\.json$/, "")


It should be before windows part as well. Or, maybe, just make windows part replace backslashes with slashes to make it cleaner?

Absolute import path slashes normalized

5f2d771

hazzard993 added 2 commits February 11, 2019 08:25

Added require spec tests

19f502a

path logic no longer uses absolute paths

c1c4fda

lolleko requested changes Feb 11, 2019

View reviewed changes

hazzard993 added 9 commits February 16, 2019 09:46

Added errors and path conversion

37cb373

Added filename for compiler and more tests

f363a12

Removed FocusTest

7d0902f

Added default root dir

d0038ab

Merging

2f14f0a

Fixed test

e9a3fbc

Fixed paths for Linux and Windows machines

45306bf

Merged with master

535c7b2

Merged

c895532

ark120202 reviewed Feb 18, 2019

View reviewed changes

.json paths are chopped

aee8991

TheLartians reviewed Feb 18, 2019

View reviewed changes

json replacement first

a8a759a

ark120202 reviewed Feb 18, 2019

View reviewed changes

json path before windows

488be23

Perryvw merged commit 9a23ce3 into TypeScriptToLua:master Feb 18, 2019

	if (process.platform === "win32") {
	filePath = filePath.replace(/\.json$/, '');
	if (process.platform === "win32") {

Uh oh!

Conversation

hazzard993 commented Feb 10, 2019

Uh oh!

Perryvw commented Feb 10, 2019

Uh oh!

hazzard993 commented Feb 10, 2019

Uh oh!

Perryvw commented Feb 11, 2019

Uh oh!

lolleko Feb 11, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

lolleko Feb 11, 2019 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lolleko Feb 11, 2019 •

edited

Loading

lolleko Feb 11, 2019 •

edited

Loading