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

Documentation of current issues with 3.1.0 #24

Open
9 of 12 tasks
dd137 opened this issue Sep 21, 2023 · 16 comments
Open
9 of 12 tasks

Documentation of current issues with 3.1.0 #24

dd137 opened this issue Sep 21, 2023 · 16 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@dd137
Copy link

dd137 commented Sep 21, 2023

I have a bit of time in the next few weeks and intend to spend some of it on meteor-desktop. My main goal would be to help make it compatible with newer Electron versions.

As I don't have preexisting knowledge of meteor-desktop nor Electron, I started with trying to set up a working dev environment for MD (trickier than I thought it would be...) and listing issues I found so far. It would be helpful if someone more experienced could give their input below. See also #13.

Unless otherwise stated, I'm running the latest official releases, i.e. meteor-desktop 3.1.0 or rather f6f824a (Electron 11.5.0), Meteor 2.3.13, Node 14.21.3, macOS Ventura.

Issues with devEnvSetup and its tests

  • There were a series of problems with devEnvSetup.js itself
  • The penultimate test of devEnvSetup, npm run desktop -- build -b in meteor-desktop-test-app/, doesn't work.
    • ERROR meteorApp: build was terminated by meteor-desktop as some errors were reported to stderr. The errors in questions are things like (node:40872) Warning: Accessing non-existent property 'cat' of module exports inside circular dependency
    • These warnings are introduced by a previous step of devEnvSetup: npm run desktop -- init-tests-support. I.e. if that step is skipped, npm run desktop -- build -b runs fine
    • Note: running npm start and npm run desktop -- build (no -b) separately works fine (with the warnings). Also, I think npm run desktop -- init-tests-support is only needed for the last test (below), not this build test.
    • Also "fixed" by Fixes for devEnvSetup.js #23
  • The last test, npm run test-desktop in meteor-desktop-test-app/ doesn't work either
    • ✘ Based on your configuration, 1 test file was found, but did not match the CLI arguments: * .desktop/**/*.test.js
    • Same thing on MD 3.0.1. Am I doing something wrong? This seems like a basic config problem.
  • mongod isn’t stopped afternpm run desktop -- build -b, which prevents running meteor again
  • Integration tests (npm run test-integration in meteor-desktop) fail

Installing meteor-desktop

  • As the currently published version (v3.1.0) isn't up to date with master (f6f824a), the app crashes if running meteor-desktop (the NPM package) locally but using the published plugins (Meteor packages), or vice-versa.
    • This is due to the asar -> @electron/asar update
    • Could we publish the current master version (ideally with the 1ea4fc0 fix) as v3.1.1? This should be a net positive. Keeping the above PR and Fix bugs in 3.1 #13 in mind for a v3.1.2
    • => Process under way for v3.1.1. See Fix bugs in 3.1 #13
  • meteor npm i meteor-desktop can't automatically add the desktop script anymore (already with 3.0.1 I think)
    • [meteor-desktop] failed to add meteor-desktop to your package.json scripts, please add it manually as 'desktop': 'meteor-desktop’
    • Fixed in Fix/add desktop script #26
  • npm i inside meteor-desktop itself causes the following error on my machine:
> puppeteer@20.8.2 postinstall /Users/dorandeluz/Documents/Work/Jobs/LirisTech/App/meteor-upgrades-2023/meteor-desktop-310/node_modules/puppeteer
> node install.js
# ...
SyntaxError: Unexpected token '??='
  • This is caused by 3.1.0 having upgraded spectron to v13.0.0, which requires puppeteer 20 which requires node >= 16.3.0. Spectron 10.0.1 is the last version working with node 14. Should we downgrade?
  • => Spectron 10 causes other issues (see comment below). Leaving Spectron 13 for now (error seems non-critical)
  • => Marking this as done as Spectron is only a dev dep and the puppeteer build error doesn't prevent even the tests from passing. Also installing MD locally with Node 16 works too.

Running meteor-desktop

Electron main process errors

Electron renderer process errors

  • Cannot read property 'BrowserWindow' of undefined at Object.exports.install (/.../devrton/api.js:6) at WebFrame.e.startsWith.e.startsWith.WebFrame.<computed> [as _executeJavaScript] (electron/js2c/renderer_init.js: 87)
    • This also appears in the main process logs: (node:21799) UnhandledPromiseRejectionWarning: Error: Script failed to execute, this normally means an error was thrown. Check the renderer console for the error. at WebFrame.e.startsWith.e.startsWith.WebFrame.<computed> [as _executeJavaScript] (electron/js2c/renderer_init.js:87:1577)
    • Fixed in Devrton: Remove all usage of and references to the obsolete package #27
  • (already in 3.0.1) If running meteor with --exclude-archs web.browser.legacy: Uncaught TypeError: Cannot read property 'versionHmr' of undefined at module (hot-module-replacement.js?hash=...:149)
    • Probably not a priority but I find it weird that we depend on web.browser.legacy
  • Multiple messages DevTools failed to load SourceMap: Could not parse content for meteor://desktop/__cordova/app/....map: Unexpected end of JSON input
@dd137 dd137 added the bug Something isn't working label Sep 21, 2023
@github-actions
Copy link

Thank you for submitting this issue!

We, the Members of Meteor Community Packages take every issue seriously.
Our goal is to provide long-term lifecycles for packages and keep up
with the newest changes in Meteor and the overall NodeJs/JavaScript ecosystem.

However, we contribute to these packages mostly in our free time.
Therefore, we can't guarantee your issues to be solved within certain time.

If you think this issue is trivial to solve, don't hesitate to submit
a pull request, too! We will accompany you in the process with reviews and hints
on how to get development set up.

Please also consider sponsoring the maintainers of the package.
If you don't know who is currently maintaining this package, just leave a comment
and we'll let you know

@a4xrbj1
Copy link

a4xrbj1 commented Sep 21, 2023

I have a bit of time in the next few weeks and intend to spend some of it on meteor-desktop. My main goal would be to help make it compatible with newer Electron versions.

Thanks Doran, that's highly appreciated!

Unless otherwise stated, I'm running the latest official releases, i.e. meteor-desktop 3.1.0 or rather f6f824a (Electron 11.5.0), Meteor 2.3.13, Node 14.21.3, macOS Ventura.

Further info, I'm using:

electron-updater 5.0.5
@electron/notarize 2.1.0
@electron/osx-sign 1.0.4
@meteor-community/meteor-desktop 3.0.1 (due to the error mentioned below with 3.1.0
electron-builder 23.1.0
electron-packager 17.1.1
electron-rebuild 3.2.9

How are you able to run Meteor 2.13.3 without Node 14.21.4?

This is caused by 3.1.0 having upgraded spectron to v13.0.0, which requires puppeteer 20 which requires node >= 16.3.0
Spectron 10.0.1 is the last version working with node 14. Should we downgrade?

I think we should downgrade. This error with version 3.1.0 is going on for many months and this will hopefully fix it. Thanks!

@dd137
Copy link
Author

dd137 commented Sep 25, 2023

Hi Andreas, thanks for the feedback!

How are you able to run Meteor 2.13.3 without Node 14.21.4?

I had meteor node -v = 14.21.4 but node -v = 14.21.3 (latest official Node version). But you're right that I should use the same versions ideally. I've now installed 14.21.4 globally on my system.

I think we should downgrade [to Spectron 10.0.1]. This error with version 3.1.0 is going on for many months and this will hopefully fix it. Thanks!

Noted. We should check that v10 doesn't hinder the tests (anymore than they are now) but I think it should be fine as 3.0.1 was using v8. If there's no other feedback from the team I'll create a PR for it.

Additionally, I looked into the following issue from my original post:

  • Cannot read property 'BrowserWindow' of undefined at Object.exports.install (/.../devrton/api.js:6) at WebFrame.e.startsWith.e.startsWith.WebFrame.<computed> [as _executeJavaScript] (electron/js2c/renderer_init.js: 87)

This is due to Devtron not being compatible with Electron 11+. See e.g. electron-userland/devtron#211. Unfortunately the project has been unmaintained for 7 years now and a post about alternatives a year ago had no replies. So my opinion is that we should sadly drop support for Devtron in meteor-desktop. If there's no opposing view on this, I can create a PR to remove all usage of/references to the tool.

Next I was thinking of looking into:

  • meteor npm i meteor-desktop can't automatically add the desktop script anymore (already with 3.0.1 I think)

as I think this is the first failed test that is currently blocking CircleCI (#13 (comment)). @StorytellerCZ if you have a different opinion on priorities, feel free to let me know, thanks.

@a4xrbj1
Copy link

a4xrbj1 commented Sep 25, 2023

This is due to Devtron not being compatible with Electron 11+. See e.g. electron-userland/devtron#211. Unfortunately the project has been unmaintained for 7 years now and electron-userland/devtron#261 a year ago had no replies. So my opinion is that we should sadly drop support for Devtron in meteor-desktop. If there's no opposing view on this, I can create a PR to remove all usage of/references to the tool.

I agree to drop support for Devtron. I too was seeing that post and unfortunately no alternative has come up.

You should go ahead and create that PR to remove all usages of/references to Devtron. Thank you!

@StorytellerCZ
Copy link
Member

StorytellerCZ commented Sep 26, 2023

@dd137 thank you for you analysis and very much appreciated that you are willing to move things forward. Best to move things one PR at a time. Let's start with merging #23 which I think is a nice start.

Spectron 10.0.1 is the last version working with node 14. Should we downgrade?

Yes, let's do that and get 3.x working first. Then we can start on v4 which will be with Meteor 3 and Node 18.

So my opinion is that we should sadly drop support for Devtron in meteor-desktop. If there's no opposing view on this, I can create a PR to remove all usage of/references to the tool.
@dd137 please go ahead.

If you can prepare as much fixes/PRs towards #13 then I'm going to merge them this weekend and release a patch version of everything so that we can proceed further.

@a4xrbj1
Copy link

a4xrbj1 commented Sep 26, 2023

Hi Jan,

Yes, let's do that and get 3.x working first. Then we can start on v4 which will be with Meteor 3 and Node 18.

In regards to next steps, as an active user of ElectronJS and Meteor-desktop I'd like if we could proceed in the following way:

  • ensure first all tests and the whole CI/CD circle is running without any errors, so we do have an error free basis to start from
  • identify how we can update ElectronJS. We're very far behind both in new features but as well in security updates
  • in line with the previous step, identify which version of ElectronJS is needed with updating the other Meteor desktop related packages like electron-updater, electron-notarize, electron-osx (for MacOS), electron-builder, electron-packager and electron-rebuild. Have I forgotten any other package? I'd be happy to test newer versions to see if they fail
  • in the meantime the Meteor 3.x and Node 18 work has hopefully advanced further, however I'd give this part the lowest priority right now. There will certainly be some issues with it and I personally don't plan to update my production app anytime soon to 3.x once it comes out. I can't afford to have any problems.

This is obviously my personal view only, I hope others can add theirs to the direction and order we should take.

@dd137
Copy link
Author

dd137 commented Sep 26, 2023

If you can prepare as much fixes/PRs towards #13 then I'm going to merge them this weekend and release a patch version of everything so that we can proceed further.

Sounds good. I'll create the PRs against the #13 branch. I should be able to get a few done in the next couple of days.

Regarding future steps (once things work well again), I'd also prefer focusing on Electron upgrades for Meteor 2 for now, unless it becomes much easier to do so for Meteor 3. We have a business-critical app that I don't think we'll be able to migrate quickly. But let's see.

@dd137
Copy link
Author

dd137 commented Sep 28, 2023

Spectron 10.0.1 is the last version working with node 14. Should we downgrade?

Unfortunately I get some weird problems (test apps open ~10 times and all crash) when running test-integration with the downgraded version. We could research further why that is (edit: actually, Spectron 10 is not expected to work with Electron 11) but since the puppeteer installation error doesn't seem to impact neither tests nor normal usage of meteor-desktop, I guess that's not a priority. So I suggest we keep using Spectron 13 for now...

  • meteor npm i meteor-desktop can't automatically add the desktop script anymore (already with 3.0.1 I think)

I had a look into that issue and hopefully fixed the problem. I'll create a PR today.

@dd137
Copy link
Author

dd137 commented Sep 28, 2023

I fixed 3-4 additional issues from the original post, the PRs are ready (thanks @StorytellerCZ).

I started looking into the second test-integration test that fails (the first one should be fixed): "expose electron modules". Unlike what I thought earlier, this is actually related to Spectron. So I'm not sure it will be fixable as long as we're not able to fully build Spectron (which probably requires Node 16 for Spectron 13, see OP). And it looks like downgrading isn't an option either (see previous post).

I will look more into this next week but I'm not sure what can be done. If someone has more experience with Electron / Spectron, that'd be great (I have none so far). On the upside, this is only relevant for testing, I don't think it affects meteor-desktop otherwise.

@StorytellerCZ
Copy link
Member

Thanks a lot @dd137 for all your amazing work!
I have removed the CicleCI test that we no longer use, so that we get more accurate test reports.
I was originally thinking of leaving spectron removal for a minor version bump, but now that I read your article, then I will merge the removal to see if it fixes the tests. After that I would like to release a patch release anyhow to get all the fixes out to people who need them.

@dd137
Copy link
Author

dd137 commented Sep 29, 2023

Thanks @StorytellerCZ! To clarify:

  • Devrton was a debug console tool. This I removed from the codebase, because not compatible with Electron 11+. It should have no impact on the tests.
  • Spectron is an Electron testing library. It is used by the CI's Integration tests (expose electron modules). Although Spectron currently doesn't work, I have not removed it from the codebase because I hope we can find a solution or an alternative. So as you saw (Fix bugs in 3.1 #13 (comment)), that test still fails, but it would also fail if we simply removed Spectron. (We could also disable that test until we find a solution)

@a4xrbj1
Copy link

a4xrbj1 commented Sep 29, 2023

With Spectron being deprecated since Feb 2022 (Source) why are we still spending time on it? Shouldn't that be enough reason to remove it completely?

I don't know which integration tests are being run by the CI/CD and if they can be ported over to the alternatives listed here: https://www.electronjs.org/docs/latest/tutorial/automated-testing

@a4xrbj1
Copy link

a4xrbj1 commented Oct 3, 2023

@StorytellerCZ and @dd137 - I've noticed that a lot of npm packages in the ".meteor/desktop-build" folder are outdated.

When I updated them (eg "connect" and "lodash") and restart my ElectronJS app all seems to be fine. However, shortly afterwards the two packages are back to their original versions.

Do you know if the "package.json" in this folder is set by the Meteor-Desktop package and if so, how can we update those outdated packages? I'd love to help on this one as some packages are way behind but would like to ask for your opinion first of all.

PS: I think these outdated packages should be added to this issue as a task

@dd137
Copy link
Author

dd137 commented Oct 4, 2023

Hey guys. Good news, I fixed the remaining integration tests. Please see #29.

With that, I think v3.1 is finally working again. There are a couple of issues still open in OP, but they are low priority in my opinion. @a4xrbj1 If you could test your app locally on branch fix/integration-tests, I'm curious if everything works for you (I have only tested skeleton meteor projects so far).

With Spectron being deprecated since Feb 2022 (Source) why are we still spending time on it? Shouldn't that be enough reason to remove it completely?

It's needed for the integration tests. In principle I agree it'd be good to port the tests to a non-legacy library but that would probably require significantly more work. Meanwhile Spectron supports Electron versions up to 17. In the end, the Spectron issue was mostly due to Meteor 2 only running with Node 14. So we might run into the same problem with other testing libraries. (In general, the problem will be more and more present as we try to upgrade Electron and other packages versions).

Do you know if the "package.json" in this folder is set by the Meteor-Desktop package and if so, how can we update those outdated packages? I'd love to help on this one as some packages are way behind but would like to ask for your opinion first of all.

I'm not sure. These deps seem to originate from lib/skeletonDependencies.js. I agree the versions in this file should be updated. But regarding your issue: I would have thought this file is only used when scaffolding a meteor-desktop app (one-time only) and I don't know how the versions get reverted afterwards. If you don't mind creating a new issue (this one was mainly meant for errors that prevent MD from building/testing/running, plus upgrading deps might create new issues) with the steps that lead to your problem, I can try to have a look into it too.

@a4xrbj1
Copy link

a4xrbj1 commented Oct 4, 2023

With that, I think v3.1 is finally working again. There are a couple of issues still open in OP, but they are low priority in my opinion.

That's great progress and indeed best news on this package so far. Thanks so much for all the hard work to get the tests going again!

@a4xrbj1 If you could test your app locally on branch fix/integration-tests, I'm curious if everything works for you (I have only tested skeleton meteor projects so far).

Not sure I fully understand. You want me to upgrade to the latest RC and run my ElectronJS app locally to see if it works still?

Sorry, I don't have much experience with npm packages but you can shoot me an email directly with my handle here (a4xrbj1) followed by gmail and explain in a bit more detail what you want me to test.

Meanwhile Spectron supports Electron versions up to 17. In the end, the Spectron issue was mostly due to Meteor 2 only running with Node 14. So we might run into the same problem with other testing libraries. (In general, the problem will be more and more present as we try to upgrade Electron and other packages versions).

Ok, given this info I agree with your view (keep Spectron), it's indeed getting more and more difficult as npm packages have moved to Node 16 and we're left behind until Meteor 3.x comes out and is stable!

These deps seem to originate from lib/skeletonDependencies.js. I agree the versions in this file should be updated. But regarding your issue: I would have thought this file is only used when scaffolding a meteor-desktop app (one-time only) and I don't know how the versions get reverted afterwards. If you don't mind creating a new issue (this one was mainly meant for errors that prevent MD from building/testing/running, plus upgrading deps might create new issues) with the steps that lead to your problem, I can try to have a look into it too.

It was a weird behavior, need to try to replicate it. So if it's for the skeleton only, there should be no problem in updating them. Will follow your suggestion to create a new issue.

@harryadel
Copy link
Member

You're on a roll @dd137 Where've you been this entire time?! Thank you for your contributions!

@StorytellerCZ StorytellerCZ added the help wanted Extra attention is needed label Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants