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

Fix frontend use of Uri.joinPath with path-browserify #10745

Merged
merged 1 commit into from
Feb 18, 2022

Conversation

thegecko
Copy link
Member

@thegecko thegecko commented Feb 13, 2022

Signed-off-by: robmor01 [email protected]

What it does

Using vscode.Uri.joinPath() or theia.Uri.joinPath() in frontend modules (Theia frontend plugins or VS Code Web Extensions) was failing because path was not found:

TypeError: Cannot read properties of undefined (reading 'join')

This PR adds the path-browserify polyfill to fix the issue.

Note, a new dependency has been added which uses the MIT license, does this need to be tracked or signed off?

path-browserify

How to test

Create a frontend module which calls Uri.joinPath()

Review checklist

Reminder for reviewers

@thegecko thegecko changed the title Frontend use of Uri.joinPath with path-browserify Fix frontend use of Uri.joinPath with path-browserify Feb 13, 2022
@vince-fugnitto vince-fugnitto added the vscode issues related to VSCode compatibility label Feb 14, 2022
@vince-fugnitto
Copy link
Member

This PR adds the path-browserify polyfill to fix the issue.
Note, a new dependency has been added which uses the MIT license, does this need to be tracked or signed off?

@thegecko the automated IP Due Diligence check passed with the new dependency, it should be good to use 👍

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thegecko from the browsersify/path-browser documentation it states that:

You usually do not have to install path-browserify yourself! If your code runs in Node.js, path is built in. If your code runs in the browser, bundlers like browserify or webpack include the path-browserify module by default.

Is the change still necessary, is it not handled by webpack?

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Infact it looks like it might be necessary based on the webpack documentation.
I confirmed that the joinPath works in web extensions:

image

@paul-marechal
Copy link
Member

@thegecko I remembered that we had a small implementation of path in the plugin code: https://github.com/eclipse-theia/theia/blob/443130396f7b1118eaf02e9309be555c938409ae/packages/plugin-ext/src/plugin/path.ts

Would that suffice or is it better to use the full npm polyfill? Just trying to figure out if we have redundancy here.

@thegecko
Copy link
Member Author

Is the change still necessary, is it not handled by webpack?

Yeah, webpack 5 no longer bundles the polyfills

I remembered that we had a small implementation of path in the plugin code

Hmm, this may work for the 2 functions defined, but there may be other path functions/cases in future. Personally I'd rather rely on the recommended polyfill.

Will merge for now, but happy to revisit using the leaner path implementation in a future PR.

@thegecko thegecko merged commit 1525243 into eclipse-theia:master Feb 18, 2022
@thegecko thegecko deleted the frontend-joinpath branch February 18, 2022 19:28
colin-grant-work pushed a commit to colin-grant-work/theia that referenced this pull request Feb 23, 2022
@vince-fugnitto vince-fugnitto added this to the 1.23.0 milestone Feb 24, 2022
federicobozzini pushed a commit to ARMmbed/theia that referenced this pull request Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants