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: adapt for npm switch in theia #44

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

planger
Copy link
Contributor

@planger planger commented Jan 9, 2025

eclipse-theia/theia#14481

Contributed on behalf of STMicroelectronics

eclipse-theia/theia#14481

Contributed on behalf of STMicroelectronics
@planger planger force-pushed the planger/adapt-to-theia-14481 branch from 1ddaf1c to ba36782 Compare January 9, 2025 12:14
@planger planger force-pushed the planger/adapt-to-theia-14481 branch from ba36782 to 0ab0420 Compare January 9, 2025 13:23
@tsmaeder
Copy link

It looks like npm hoists the electron package and binary to the root node_modules folder instead of the examples/electron. The application manager starts electron like this: This shoudl work, IMO.

        const electronCli = require.resolve('electron/cli.js', { paths: [this.pck.projectPath] });
        return this.__process.fork(electronCli, mainArgs, options);

@planger
Copy link
Contributor Author

planger commented Jan 14, 2025

Thanks for looking into that @tsmaeder! Does this need to be adapted?

https://github.com/eclipse-theia/theia-e2e-test-suite/blob/main/tests/theia-app.test.ts#L23

test.beforeAll(async ({ playwright, browser }) => {
    app = await TheiaAppLoader.load({
        playwright, useElectron: { electronAppPath: 'theia/examples/electron', }, browser
    });
});

@tsmaeder
Copy link

No but there is a place where we compute the path to the electron executable relative to the electron app path. theia-app-loader.ts#127

@planger
Copy link
Contributor Author

planger commented Jan 14, 2025

Ah ok, so it needs to be fixed in Theia, right? Can you please take care of the fix or should I? Thanks!

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 this pull request may close these issues.

2 participants