Skip to content

Use default import of fix-path package #14812

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.

Sign up for GitHub

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

Jump to bottom
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

@msujew msujew requested a review from jfaltermeier February 2, 2025 22:49
@msujew msujew mentioned this pull request Feb 2, 2025
Closed
@rschnekenbu
Copy link
Contributor

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

kittaakos
kittaakos approved these changes Feb 3, 2025
View reviewed changes
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

jfaltermeier
jfaltermeier approved these changes Feb 3, 2025
View reviewed changes
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 •
edited
Loading

@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 ;)

jfaltermeier pushed a commit that referenced this pull request Feb 3, 2025
@msujew @jfaltermeier
Use default import of fix-path package (#14812)
39f6502
@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.

@jfaltermeier jfaltermeier mentioned this pull request Feb 3, 2025
Closed
2 tasks
marcdumais-work added a commit to eclipse-cdt-cloud/theia-trace-extension that referenced this pull request Apr 8, 2025
@marcdumais-work
Update for Theia community release v1.58.4
9700ed0 
Issue encountered:
eclipse-theia/theia#14804

Another PR updated "fix-path" and caused the issue:
eclipse-theia/theia#14781

A fix was done:
eclipse-theia/theia#14812

Side-effect of the fix:
eclipse-theia/theia#14804 (comment)

Fix to address the side-effect:
eclipse-theia/theia#14819

Note: the fix for the side-effect was merged on Theia master,
and does not help momentariry on the community release 1.58.x,
on which it's not currently planned to back-port this fix:
eclipse-theia/theia#14819 (comment)

We could contribute a PR to back-port the fix, but I do not
think this is super important for us - the Theia Electron app
is only an example application and permanently switching to
a bundled Electron backend does not seem to have any downside.

In consequence, this PR includes using the bundled Electron
backend, which is a simple webpack config update. If we later
notice issues with this, we can roll-back to use the non-bundled
backend in the next community release, which will have the fix
above.

Note about the webpack config update: as suggested, I have
deleted the webpack configs and run "theia build" to have
them re-created. Then I re-added the small change we need
for the time range context menu. The result is a webpack
config that's more aligned with the one Theia generates,
for both our example Theia apps.

Signed-off-by: Marc Dumais <marc.dumais@ericsson.com>
marcdumais-work added a commit to eclipse-cdt-cloud/theia-trace-extension that referenced this pull request Apr 8, 2025
@marcdumais-work
Update for Theia community release v1.58.4
54c2951 
Issue encountered:
eclipse-theia/theia#14804

A PR updated "fix-path" and caused the issue:
eclipse-theia/theia#14781

A fix was done:
eclipse-theia/theia#14812

Side-effect of the fix:
eclipse-theia/theia#14804 (comment)

Fix to address the side-effect:
eclipse-theia/theia#14819

Note: the fix for the side-effect was merged on Theia master,
and does not help momentariry on the community release 1.58.x,
on which it's not currently planned to back-port this fix:
eclipse-theia/theia#14819 (comment)

We could contribute a PR to back-port the fix, but I do not
think this is super important for us - the Theia Electron app
is only an example application and permanently switching to
a bundled Electron backend does not seem to have any downside.

In consequence, this PR includes using the bundled Electron
backend, which is a simple webpack config update. If we later
notice issues with this, we can roll-back to use the non-bundled
backend in the next community release, which will have the fix
above.

Note about the webpack config update: as suggested, I have
deleted the webpack configs and run "theia build" to have
them re-created. Then I re-added the small change we need
for the time range context menu. The result is a webpack
config that's more aligned with the one Theia generates,
for both our example Theia apps.

Signed-off-by: Marc Dumais <marc.dumais@ericsson.com>
marcdumais-work added a commit to eclipse-cdt-cloud/theia-trace-extension that referenced this pull request Apr 14, 2025
@marcdumais-work
Update for Theia community release v1.58.5
2828481 
Issue encountered:
eclipse-theia/theia#14804

A PR updated "fix-path" and caused the issue:
eclipse-theia/theia#14781

A fix was done:
eclipse-theia/theia#14812

Side-effect of the fix:
eclipse-theia/theia#14804 (comment)

Fix to address the side-effect:
eclipse-theia/theia#14819

Note: the fix for the side-effect was merged on Theia master,
and does not help momentariry on the community release 1.58.x,
on which it's not currently planned to back-port this fix:
eclipse-theia/theia#14819 (comment)

We could contribute a PR to back-port the fix, but I do not
think this is super important for us - the Theia Electron app
is only an example application and permanently switching to
a bundled Electron backend does not seem to have any downside.

In consequence, this PR includes using the bundled Electron
backend, which is a simple webpack config update. If we later
notice issues with this, we can roll-back to use the non-bundled
backend in the next community release, which will have the fix
above.

Note about the webpack config update: as suggested, I have
deleted the webpack configs and run "theia build" to have
them re-created. Then I re-added the small change we need
for the time range context menu. The result is a webpack
config that's more aligned with the one Theia generates,
for both our example Theia apps.

Signed-off-by: Marc Dumais <marc.dumais@ericsson.com>
marcdumais-work added a commit to eclipse-cdt-cloud/theia-trace-extension that referenced this pull request Apr 15, 2025
@marcdumais-work
Update for Theia community release v1.58.5
3802bd2 
Issue encountered:
eclipse-theia/theia#14804

A PR updated "fix-path" and caused the issue:
eclipse-theia/theia#14781

A fix was done:
eclipse-theia/theia#14812

Side-effect of the fix:
eclipse-theia/theia#14804 (comment)

Fix to address the side-effect:
eclipse-theia/theia#14819

Note: the fix for the side-effect was merged on Theia master,
and does not help momentariry on the community release 1.58.x,
on which it's not currently planned to back-port this fix:
eclipse-theia/theia#14819 (comment)

We could contribute a PR to back-port the fix, but I do not
think this is super important for us - the Theia Electron app
is only an example application and permanently switching to
a bundled Electron backend does not seem to have any downside.

In consequence, this PR includes using the bundled Electron
backend, which is a simple webpack config update. If we later
notice issues with this, we can roll-back to use the non-bundled
backend in the next community release, which will have the fix
above.

Note about the webpack config update: as suggested, I have
deleted the webpack configs and run "theia build" to have
them re-created. Then I re-added the small change we need
for the time range context menu. The result is a webpack
config that's more aligned with the one Theia generates,
for both our example Theia apps.

Signed-off-by: Marc Dumais <marc.dumais@ericsson.com>
laemmleint pushed a commit to mvtecsoftware/theia that referenced this pull request Aug 18, 2025
@msujew @laemmleint
Use default import of fix-path package (eclipse-theia#14812)
d56baa2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfaltermeier jfaltermeier jfaltermeier approved these changes

+1 more reviewer

@kittaakos kittaakos kittaakos approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
PR Backlog
Archived in project
Milestone
1.59.0
Development

Successfully merging this pull request may close these issues.

The generated sample Electron app crashes on startup
5 participants
@msujew @rschnekenbu @sdirix @kittaakos @jfaltermeier