Fix symlinks should be resolved when initializing plugins

[vatsal-uppal-1997]

What it does

Fixes: #12840

How to test

Follow the steps mentioned in #12840 to reproduce the issue

To test the fix I did a quick and dirty approach where I compiled "plugin-vscode-init.ts" in my fix repo and replaced that file in the issue repo's node_modules. The process would like as follows:

• In the fix repo do a "yarn build" inside the "plugin-ext-vscode" directory
• Copy the "plugin-ext-vscode\lib\node\plugin-vscode-init.js" and paste it in the bug repositories node_modules "node_modules\@Theia\plugin-ext-vscode\lib\node"

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[martin-fleck-at]

Hi @vatsal-uppal-1997, thank you very much for that change! I had a look at it and it works as expected given the example from the bug report.

My only concern is that I'm thinking about applying this fix a bit earlier as otherwise we need to apply it to all our custom API handling, i.e., we should at least also apply it to backend-init-theia.ts where the same mechanism is applied as in plugin-vscode-init.ts.

Just to give a bit of context: We have different plugin resolvers (PluginDeployerResolver) that can add plugins to a resolution context (PluginDeployerResolverContext) where they are transformed to plugin entries (PluginDeployerEntry) that are further modified by the file and directory handlers (PluginDeployerDirectoryHandler and PluginDeployerFileHandler). Those handlers in particular can still modify the path of the entry. Afterwards, we read the manifest from each plugin using the final path which ends up in the Plugin that you are adapting. I believe another good point to do apply this fix would be during the translation from the pluginPath to the PluginPackagein the HostedPluginReader.readPackage method. I did some initial tests and it all looks good to me. @tsmaeder Do you have an opinion on this?

[tsmaeder]

@martin-fleck-at the existence of better fix should not prevent us from merging a PR in general. The questions we ask should be: "does this make the code base better" and "does this prevent us from doing anything in the future". Why not merge it and then move it to a different place in a follow-up?

[msujew]

@vatsal-uppal-1997 Do you plan on getting back to this to integrate the review comments? Otherwise, I'll close this PR in a few days.

[vatsal-uppal-1997]

@vatsal-uppal-1997 Do you plan on getting back to this to integrate the review comments? Otherwise, I'll close this PR in a few days.

Hi @msujew,

Sorry it has been busy for me for quite a while, I'll try to look into this PR this weekend. Thanks for being patient!

[vatsal-uppal-1997]

Hi @martin-fleck-at,

I've made the changes as per your suggestions. Requesting you to please review them to see if they're satisfactory.

(++ @msujew )

[vatsal-uppal-1997]

Hi @msujew,

Thanks for your suggestions! I've removed the unnecessary changes from the files you pointed out.

I also tested the change and it seems that I only needed to modify the readPackage method of HostedPluginReader

[martin-fleck-at]

@vatsal-uppal-1997 Thank you for the update! The change looks good to me now and the problem seems to be resolved. If everyone else is also happy with this, we can merge it.

[msujew]

Looks good to me as well 👍

[kittaakos]

Can somebody please explain why the #12841 (comment) was marked as resolved, but eventually, the changes got into the master with a sync FS call?

[martin-fleck-at]

@kittaakos I'm not sure I follow, the merged commit (525b3b1) uses an async realpath call to properly set the packagePath which is why this is then no longer needed later on.

[vatsal-uppal-1997]

Hi @kittaakos,

I had removed the sync calls based on your and @msujew comments in this commit

[kittaakos]

@kittaakos I'm not sure I follow, the merged commit (525b3b1) uses an async realpath call to properly set the packagePath which is why this is then no longer needed later on.

Hi @kittaakos,

I had removed the sync calls based on your and @msujew comments in this commit

You're both great! Thanks! I confirm I was wrong; the sync call was not part of the merged PR.

---

I checked the PR today; I did not see the sync call. I happily put my 👍 to #12841 (review). Then I rechecked the PR, and I saw the sync call. I was either blind (most likely), or a cached state had loaded in my browser.