Resolving paths using path aliases about skott HOT 10 CLOSED

Hello @kevin-st,

Just published [email protected] that should fix the issue. Let me know if it works for you and if not feel free to re-open the issue.

Thanks again for submitting the issue and providing such detailed information.

Hi @antoine-coulon,

I'm so sorry for my late response; with the holidays kicking in I completely forgot about the issue. I just checked the update and everything seems to be working as expected.

Thanks for taking the time to look at this!

from skott.

Hey @kevin-st, thanks for reporting the issue and providing that very detailed repro.

I've seen #124 which seems to be kind of the same thing, but I'm not certain. I haven't heard yet of "import maps" so in case I'm talking about the same issue here, feel free to mark it as duplicate.

It's not the same issue so you were right to open it, the #124 is about supporting Node.js path aliases while the issue you're encountering is only specific to TypeScript path aliases.

I'm under the impression that the wrong information is being used to resolve the paths. I'd think that it should be able to use the information from the registration (being @ar which is mapped to packages\ar) and replace that in the import statement when trying to resolve it.

This is exactly how it's supposed to work (and how actually it works for non-Windows environments).

From the very quick investigation I did the issue only seems related to Windows environment where paths are handled in a different way. On MacOS, I was not able to reproduce the problems you mentioned on your repo, could you please confirm me that it's the same for you if you have a Mac near by? I'll create a StackBlitz sandbox tomorrow to provide you an example to be sure that this is only a Windows problem.

The issue seems to come from the resolution step because paths aliases registered are weirdly mapped to anti slashes hence the resolver does not consider @ar/components/ARComponent to be a path alias as it's not found in the TS path aliases registry. That's why it then attempts to map packages\@ar\components\ARComponent\index.js etc on the real-file system, the path was not considered as a path alias.

So essentially I think that this is not related to the symbols @ / ~ or whatever, only due to the fact that the module import path does not match any path aliases for some Windows specific reason even though this should be supported as this is purely TS path aliases.

I'll investigate in more details tomorrow, I'll keep you updated.

from skott.

From the very quick investigation I did the issue only seems related to Windows environment where paths are handled in a different way. On MacOS, I was not able to reproduce the problems you mentioned on your repo, could you please confirm me that it's the same for you if you have a Mac near by? I'll create a StackBlitz sandbox tomorrow to provide you an example to be sure that this is only a Windows problem.

We have checked it on a Mac and it's working there as expected, so it seems to be a Windows issue indeed. I also tried to run it on WSL (Windows Subsystem for Linux), so I would expect it to run the same as on Mac, but it seems as if something's wrong there as well and that could be a separate issue.

Regarding this issue, we've been trying to look into it, but we haven't found the cause yet. However, I can share the following:

• Given the path @ar/components/ARComponent;

We see that the code is coming in the EcmaScriptDependencyResolver.resolve-method, in which several checks are being made. The check which seems to be going wrong on Windows is the isTypeScriptPathAlias function, which is resolving to false on Windows, but to true on Mac.

On Windows, we see that the isTypeScriptRelativePathWithNoLeadingIdentifier check is resolving to true, which is the reason why we eventually see in the logs that it's trying to find the files at packages\@ar\components\ARComponent.

Following the isTypeScriptPathAlias function, we see the arguments passed to the function as follows:

moduleDeclaration: @ar/components/ARComponent
aliasLinks: Map(3) {
  '@ar' => 'packages\\ar',
  '@configurator' => 'packages\\configurator',
  '@xmlParser' => 'packages\\xml-parser'
}

and pathSegmentToMatch is set to @ar/components.

That value is not set in the map, so we reach the while-loop in the function. This while-loop is never executed though, as isNotBasePathSegment resolves to false. It should resolve to true however, since we have a path seperator in pathSegmentToMatch. If we want the loop to execute, we should invert the first part of the condition:

while (!isNotBasePathSegment(pathSegmentToMatch) && pathDepthAttempts < 10)

Note: Personally, I think the isNotBasePathSegment name is a bit confusing, maybe it would be better to rename this to isChildPathSegment, or the return-statement could be inverted and then you can name the function isBasePathSegment, which makes it less confusing I think.

After making this change, the function returns true. However, something else seems to go wrong if we do it like this and from there on we currently have no clue yet what the issue exactly is.

Maybe this gives some insight.

from skott.

Thanks for the detailed analysis, it confirms the hypothesis that I had even though I didn't have time to deep dive, problem is I need to prepare few meetups, one this week and the other one next week, so I'm not really sure if I'll have time to fix the issue until middle of next week.

You have already spent a lot of time deeply investigating through that issue and I appreciate it, if you ever find a fix that solves the issue while keeping all the tests to green (+ the one that will be added to cover the current case) I'll be able to merge/publish the fix in the meantime. Otherwise I'll take the lead fix the issue asap, I have a Windows computer that I can use to troubleshoot the issue, it will be easier for me to fix it, for now it's only speculation on my side.

I also take notes about the naming suggestions there :)

Feel free to keep me updated if you guys ever find other interesting stuff to share.

Thanks

from skott.

I've been taking a look further into the issue and the reason why isNotBasePathSegment is returning false is because the function is checking if the given path contains a path.sep, which on Windows is a backslash and not a forward slash. However, since all of the import statements are written with forward slashes, it does not find the separator and thus the function returns false.

I tried to track down where the import statements are being collected and I found out that a possible location to fix the issue is in the extractModuleDeclarations function.

The way I see it, a possible solution is to check if the current platform is Windows and then making sure that the paths are formatted correctly. For example, if the import statement is written like

import ARComponent from "@ar/components/ARComponent";

The collected path would be @ar/components/ARComponent. However, if we're on Windows, we can replace the forward slashes with a backslash, which would make the collected path be @ar\\components\\ARComponent.

From then on the isNotBasePathSegment function would return true and the isTypeScriptPathAlias is resolving correctly, making the graph contain all nodes.

The only issue(?) I'm seeing here is that there is no arrow from the ARComponent to ar.handler, but I don't know if that's intended behaviour or not. If not, then that's a separate issue I guess.

The current changes that I made are as follows:

This way the changes are minimal and everything should work correctly on unix systems as nothing has changed for those platforms.

However, I'm not sure under which circumstances the other if-statements in this functions are triggered, so I don't know if they work correctly on Windows or not.

from skott.

Hello @kevin-st,

Great job investigating! As previously said I didn't have time myself to do it and I won't have time before tomorrow, but I highly appreciate your analysis.

To be honest I'm kind of confused that cross platform path issue was not covered yet, I might have mis-used the API in some kind of way, replacing double slashes might be one solution or maybe avoid relying on path.sep in the first place.

The fact that you don't have an link (arrow) where it should means that there is a mismatch between the graph representation and the module resolution so the relationship won't be established. The node's path needs to be exactly the same as when it's resolved and when it's put in the graph (both for the node and the directed link), otherwise it won't show up. Consequently resolving correctly the alias is one thing, but then we should map and add the real path (existing in the alias map) within the graph.

However, I'm not sure under which circumstances the other if-statements in this functions are triggered, so I don't know if they work correctly on Windows or not.

Unit tests are there for that normally, if that's not enough yet it means that I might need to add Windows machine on the matrix to be sure that they are all running properly on Windows as well. The outcome when running test:unit in the skott repository should be the same as on your machine.

Do you have that repro nearby that I could locally pull for the latest example? I'll start setting up my Windows machine and help you out resolving the thing.

from skott.

@kevin-st as shown in this workflow Windows platform has some unit tests failing (not even skott package yet) so I would not be amazed that some of the unit tests from skott fail on Windows (that's a good thing). I need to catch up pretty quick with Windows support, I have to admit that it is entirely my bad, I forgot to add Windows in the matrix-os so all features might not be supported/working as expected.

Starting tomorrow I'm catching up this subject and other things at the same time to make skott entirely cross platform for all kind of features.

from skott.

Hello @kevin-st,

Just opened a pull request (#128) that will, I hope, fix all windows-related issues, including the one you described there.
Implementation is mostly done, I just need to add changelogs and publish changed packages. Just FYI the fix will be included in [email protected].

Here is an overview of the graph I got with skott's version from the pull request based on the repro you provided me:

It now seems to be behaving as expected, correctly following TypeScript's path aliases.

I'll let you know when the release is published so that you can test on your own.

from skott.

Hello @kevin-st,

Just published [email protected] that should fix the issue. Let me know if it works for you and if not feel free to re-open the issue.

Thanks again for submitting the issue and providing such detailed information.

from skott.

Hello @kevin-st, no worries, no news is good news with open-source.

Thanks for letting me know I'm glad it works now, don't hesitate to re-open issues if you still encounter some problems (related to this or not) :)

from skott.