Meteor-Community-Packages / meteor-desktop

Build a Meteor's desktop client with hot code push.
MIT License
37 stars 12 forks source link

Documentation of current issues with 3.1.0 #24

Open dd137 opened 1 year ago

dd137 commented 1 year ago

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 https://github.com/Meteor-Community-Packages/meteor-desktop/pull/13.

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

Issues with devEnvSetup and its tests

Installing meteor-desktop

Running meteor-desktop

Electron main process errors

Electron renderer process errors

github-actions[bot] commented 1 year ago

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 commented 1 year ago

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 https://github.com/Meteor-Community-Packages/meteor-desktop/commit/f6f824ac58dda075c2a43fa5370e75dff216e496 (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 commented 1 year ago

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. https://github.com/electron-userland/devtron/issues/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 (https://github.com/Meteor-Community-Packages/meteor-desktop/pull/13#issuecomment-1537502790). @StorytellerCZ if you have a different opinion on priorities, feel free to let me know, thanks.

a4xrbj1 commented 1 year ago

This is due to Devtron not being compatible with Electron 11+. See e.g. https://github.com/electron-userland/devtron/issues/211. Unfortunately the project has been unmaintained for 7 years now and https://github.com/electron-userland/devtron/issues/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 commented 1 year ago

@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 commented 1 year ago

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:

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

dd137 commented 1 year ago

If you can prepare as much fixes/PRs towards https://github.com/Meteor-Community-Packages/meteor-desktop/pull/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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Thanks @StorytellerCZ! To clarify:

a4xrbj1 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

Hey guys. Good news, I fixed the remaining integration tests. Please see https://github.com/Meteor-Community-Packages/meteor-desktop/pull/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 commented 1 year ago

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 commented 1 year ago

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