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

Use default import of fix-path package #14812

Merged
merged 1 commit into from
Feb 3, 2025
Merged

Use default import of fix-path package #14812

merged 1 commit into from
Feb 3, 2025

Conversation

msujew
Copy link
Member

@msujew msujew commented Feb 2, 2025

What it does

Closes #14804

Correctly uses the default import of the fix-path library. Since the 4.0.0 version is an ESM library, we need to call default() explicitly. Additionally adjusts the imports used in the generated code to ensure that non-hoisting of libraries does not result in errors.

How to test

  1. Clean your Theia node_modules, ideally using something like git clean -xfd
  2. Run npm i && npm run build:electron && npm run start:electron
  3. Everything should work as expected.

Review checklist

Reminder for reviewers

@rschnekenbu
Copy link
Contributor

Electron app starts well with this patch. Thanks @msujew!

Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

Thank you! I verified it with 0d902cc, and it worked well.


From the main branch (17d4667), I received the same error when I ran npm run all:

ERROR in ./src-gen/backend/electron-main.js 9:0-19
Module not found: Error: Can't resolve 'fix-path' in '/Users/kittaakos/dev/git/theia/examples/electron/src-gen/backend'
resolve 'fix-path' in '/Users/kittaakos/dev/git/theia/examples/electron/src-gen/backend'
  Parsed request is a module
  using description file: /Users/kittaakos/dev/git/theia/examples/electron/package.json (relative path: ./src-gen/backend)
    resolve as module
      /Users/kittaakos/dev/git/theia/examples/electron/src-gen/backend/node_modules doesn't exist or is not a directory
      /Users/kittaakos/dev/git/theia/examples/electron/src-gen/node_modules doesn't exist or is not a directory
      /Users/kittaakos/dev/git/theia/examples/electron/node_modules doesn't exist or is not a directory
      /Users/kittaakos/dev/git/theia/examples/node_modules doesn't exist or is not a directory
      looking for modules in /Users/kittaakos/dev/git/theia/node_modules
        single file module
          using description file: /Users/kittaakos/dev/git/theia/package.json (relative path: ./node_modules/fix-path)
            no extension
              /Users/kittaakos/dev/git/theia/node_modules/fix-path doesn't exist
            .js
              /Users/kittaakos/dev/git/theia/node_modules/fix-path.js doesn't exist
            .json
              /Users/kittaakos/dev/git/theia/node_modules/fix-path.json doesn't exist
            .wasm
              /Users/kittaakos/dev/git/theia/node_modules/fix-path.wasm doesn't exist
        /Users/kittaakos/dev/git/theia/node_modules/fix-path doesn't exist
      /Users/kittaakos/dev/git/node_modules doesn't exist or is not a directory
      /Users/kittaakos/dev/node_modules doesn't exist or is not a directory
      /Users/kittaakos/node_modules doesn't exist or is not a directory
      /Users/node_modules doesn't exist or is not a directory
      /node_modules doesn't exist or is not a directory

Copy link
Contributor

@jfaltermeier jfaltermeier left a comment

Choose a reason for hiding this comment

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

LGTM, the Electron app starts as expected.

@msujew msujew merged commit 424c048 into master Feb 3, 2025
11 checks passed
@msujew msujew deleted the msujew/fix-path branch February 3, 2025 09:02
@github-actions github-actions bot added this to the 1.59.0 milestone Feb 3, 2025
@sdirix
Copy link
Member

sdirix commented Feb 3, 2025

@msujew @jfaltermeier Note that this a webpack-based solution if I'm not mistaken. So we should mark in the changelog that bundling the backend is now mandatory, as otherwise the Electron startup will fail.

@msujew
Copy link
Member Author

msujew commented Feb 3, 2025

@sdirix Um, yes you're right. However, I believe that without a bundler, the backend fails to load anyway, since the fix-path cannot be required in standard node.js, as it is a ESM library.

@tsmaeder All the more reason to switch to ESM ;)

@sdirix
Copy link
Member

sdirix commented Feb 3, 2025

@msujew The previously used fix-path was commonjs, so there it worked. We got into the problem now with the fix-path upgrade as it is now an ESM version. So it's not the consequence of this PR, but the one which did the upgrade that we now require bundling. Either way, it should be mentioned as a breaking change for the new version of Theia.

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

Successfully merging this pull request may close these issues.

The generated sample Electron app crashes on startup
5 participants