Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

module load redirection logic in @theia/plugin-ext-vscode misbehaves in the presence of a symlink #12840

Closed
jcortell68 opened this issue Aug 18, 2023 · 4 comments · Fixed by #12841

Comments

@jcortell68
Copy link
Contributor

jcortell68 commented Aug 18, 2023

This happens only in the development environment of someone writing a VS Code extension for Theia. I don't think it happens in a deployed installation of the app, since I don't think there's a symlink at play there. The Theia documentation recommends the use of a plugin symlink in the monorepo. See the recommendation here.

Bug Description:

The problem is in the super simple implementation of the local utility function findPlugin here

That module keeps a cache of API objects--one for each VS Code extension the Theia app loads. The idea is that if VS Code extension XYZ has 50 modules which each do, e.g.,

import * as vscode from 'vscode';

all 50 imports will end up getting the same (cached) module exports object.

The problem is that findPlugin is too simple and doesn't consider the duplicity that can happen when symlinks are involved. The Plugin.pluginFolder uses the symlink; however the parent object passed into the module._load() function does not use the symlink.

By example:

Plugin.pluginFolder=C:\git\mytheiaapp\plugins\myext
parent.filename=C:\git\mytheiaapp\myext\dist\extension.js

Note that plugins is not present in the latter, thus findPlugin returns undefined.

I fear there might be functional problems that results from this. Consider that the API factory takes a Plugin object. And if we look at the factory implementation, we see the plugin argument is used to create many parts of the API. When the match described above fails due to the symlink, the extension is given a default singleton API object object that is created using emptyPlugin, thus lacking context that may be necessary for the vscode API implementation to work correctly. And in fact, I wonder if that's not the root cause of #12828

Steps to Reproduce:

git clone -b vscode_extension https://github.com/jcortell68/theiasandbox
cd theiasandbox
yarn
yarn start:electron

Invoke the Hello World command contribution.

In the stdout, you'll see:

2023-08-18T15:01:45.196Z root ERROR [hosted-plugin: 31032] Could not identify plugin for 'Theia' require call from C:\<your-path>\theiasandbox\myext\dist\extension.js

Additional Information

  • Operating System: Windows
  • Theia Version: 1.39
@vatsal-uppal-1997
Copy link
Contributor

vatsal-uppal-1997 commented Aug 18, 2023

Hi @jcortell68,

How about the following fix for this issue:

Change

In the above change, I am using nodejs' "realpathSync" FS API to resolve any symlinks in the plugin folder to their actual path which later would be used by the "findPlugin" function.

Do you perhaps see any downsides to this approach? If not I'll raise a PR and hopefully this issue would be resolved

Edit: This seems to fix #12828 as you suggested

image

@jcortell68
Copy link
Contributor Author

@vatsal-uppal-1997 Works like a charm. Thanks! Assuming that parent passed into the module loader always has a canonicalized path, this seems like the perfect fix. If there is any chance that's not the case, I suppose we'd want a fallback, second check in findPlugin() that does realpathSync(filePath). But lacking any evidence or even suspicion, I say let's go with your succinct solution.

@jcortell68
Copy link
Contributor Author

jcortell68 commented Aug 18, 2023

Edit: This seems to fix #12828 as you suggested

NICE! I've closed that ticket as it is effectively a duplicate.

@vatsal-uppal-1997
Copy link
Contributor

@vince-fugnitto Raised a PR to fix this issue, requesting a review:

#12841

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants