Fix issue #2295 - Models with "@" in their name do not resolve as dependencies

[bsorrentino]

fix issue #2295

component: TypeScriptWorker (tsWorker.ts)

• Enable 'skipEncoding' flag on Uri.toString invokation on getScriptFileNames() method
• Compare 'fileName' argument provided to _getModel() method both with Uri encoded and not

[hediet]

Thanks for the PR and your investigation!

I don't fully understand the problem though, why does this fix the issue?

[bsorrentino]

Thanks for the PR and your investigation!

I don't fully understand the problem though, why does this fix the issue?

Hi @hediet thanks for starting review

I tackled issue #2295 integrating monaco editor in my app.

As you could see from issue's comments the problem was related to typescript language when we import the scoped package (@scope/package)

@lstkz has figure out that using %40 instead of @ the problem disappear ( e.g. using %40azure/msal instead of @azure/msal ) as states in this comment

With this hint I've investigated on the part that resolve the import and finally I got a fix.

Hope this clarify bit more

[hediet]

That I understand, by I don't understand the underlying issue.

I think the actual issue is that it is unclear how fileName should be encoded in private _getModel(fileName: string).

In this PR, I don't like the line if (uri.toString() === fileName || uri.toString(true) === fileName) { very much. Why do we need to check for both?

[bsorrentino]

That I understand, by I don't understand the underlying issue.

I think the actual issue is that it is unclear how fileName should be encoded in private _getModel(fileName: string).

In this PR, I don't like the line if (uri.toString() === fileName || uri.toString(true) === fileName) { very much. Why do we need to check for both?

Hi @hediet

The reason is that uri.toString() by default encode uri content itself. This for scoped packages cause that the @ char is converted to %40 and, in such case, the condition never match.

I've added the condition with uri.toString(true) in OR to be backward compatibile and allow matching condition

[pietrovismara]

Would be awesome to get this merged...

[peterp]

@bsorrentino Throwing my two cents in here because this seems to be blocked.

I don't think the code clearly communicates why the two conditional checks are important, and your comment clarified the reasoning, but I get the sense that it would help to add that as a comment so that future readers of the code also understand why this was required.

Perhaps that may give @hediet or @alexdima a bit more confidence to merge this.

[bsorrentino]

@bsorrentino Throwing my two cents in here because this seems to be blocked.

I don't think the code clearly communicates why the two conditional checks are important, and your comment clarified the reasoning, but I get the sense that it would help to add that as a comment so that future readers of the code also understand why this was required.

Perhaps that may give @hediet or @alexdima a bit more confidence to merge this.

Hi @peterp the suggestion here is to renew the PR adding meaningful comments to the affected code ?

[hrgdavor]

cmon. why is this PR still blocked ?

[hediet]

After some exploration, I also don't have a better idea than the change suggested in this PR.