angular / zone.js

Implements Zones for JavaScript
https://github.com/angular/angular/tree/master/packages/zone.js/
MIT License
3.25k stars 407 forks source link

Implement electron-zone #537

Closed robwormald closed 6 years ago

robwormald commented 7 years ago

Electron uses both browser and some node APIs (EventEmitter, etc), and events fired by electron APIs (MenuItem, for example) cause angular to drop out the zone. Ref https://github.com/angular/angular/issues/13363

We should implement an electron zone patch, that combines the browser APIs as well as a subset of the node APIs available to electron.

JiaLiPassion commented 7 years ago

I tried the issue in this repo, but can't reproduce. But I think current zone.js did not patch browser and node.js at the same time, just like #524 maybe the same issue in electron. Could you provide a reproduce step?

robwormald commented 7 years ago

@JiaLiPassion navigate via the menu at the top of the screen:

screen shot 2016-12-11 at 10 25 07 pm
JiaLiPassion commented 7 years ago

I cleaned up my node_modules, I can reproduce now.

sittingbool commented 7 years ago

any update on this? i can use angular 2 only up to version 2.1 so far with electron

JiaLiPassion commented 7 years ago

For now, you have to do like this

 let pageMenu = new MenuItem({
            label: 'Page',
            submenu: [
                { label: 'Page 1', click: Zone.current.wrap(() => this.router.navigate(['/page1'])) },
                { label: 'Page 2', click: Zone.current.wrap(() => this.router.navigate(['/page2'])) },
            ],
        });
sittingbool commented 7 years ago

I don't only have a problem with the router. I have issues in general like with change detection and such.

JiaLiPassion commented 7 years ago

@sittingbool, could you give an example through some sample code or repository.

JiaLiPassion commented 7 years ago

@sittingbool , if your problem is related with electron menu or something like that, you could use the method just like

 let pageMenu = new MenuItem({
            label: 'Page',
            submenu: [
                { label: 'Page 1', click: Zone.current.wrap(() => this.router.navigate(['/page1'])) },
                { label: 'Page 2', click: Zone.current.wrap(() => this.router.navigate(['/page2'])) },
            ],
        });

If your problem is related with your application(not electron), you may check the #533 to see whether it is the same problem, if it is , you can check PR #608 to see whether it resolve your problem or not.

pamtbaau commented 7 years ago

Any progress on this issue with electon menus and zone?

sittingbool commented 7 years ago

I really would appreciate that too I cant really use electron with Angular 2+ so far.

pamtbaau commented 7 years ago

@sittingbool The suggested workarround of wrapping my code with zone.run(...) works fine for me, but I would rather do it without...

This is how my code looks like when invoking an Electron menu item:

private initMenu() {
   const template: Electron.MenuItemConstructorOptions[] = [
      {
          label: 'Set exchange rates',
          click: () => this.zone.run(() => this.showExchangeRateDialog = true),
      },
      {
          label: 'Set stock prices',
          click: () => this.zone.run(() => this.showStockPriceDialog = true),
      },
   ];

   this.menu = Menu.buildFromTemplate(template);
}

ChangeDetector is now picking up any changes of this.showExchangeRateDialog and this.showStockPriceDialog and the dialogs will show accordingly.

sittingbool commented 7 years ago

I don't have this Issue with menu alone. I have a general Issue with the Zone- Implementation in electron. The behavior has been overwritten from the default as far as I could see and Zone.js is reacting allergic to it.

JiaLiPassion commented 7 years ago

@sittingbool , could you post a reproduce repo?

JiaLiPassion commented 7 years ago

@pamtbaau , if it is just menu issue, I can make a patch to do it, but I don't know there are other cases or not.

pamtbaau commented 7 years ago

@JiaLiPassion, So far, I've encountered the problem when setting a property inside an Electron initiated callback. E.g. a (popup)menu callback or dialog callback. I've created a repo to reproduce the three scenarios.

daganchen commented 6 years ago

Having the same issues like @sittingbool, all events are firing but component not rendering as expected. I'm using Angular5, if I downgrade to version 2, issues will stop appearing?

JiaLiPassion commented 6 years ago

@daganchen , this issue has nothing to do with angular version, I am implementing an electron zone now, please wait for a while.

ameagol commented 6 years ago

Hi All, I've just found a solution that works for Angular 5 and all latest Codes for Node.js and Typescript:

Follow:

npm install ngx-electron --save

then, use the code as follow

import { ElectronService } from 'ngx-electron';
import { Component, OnInit } from '@angular/core';
import { Router } from '@angular/router'; 

@Component({
  selector: 'app-home',
  templateUrl: './home.component.html',
  styleUrls: ['./home.component.css']
})
export class HomeComponent implements OnInit {

  constructor(private _electronService: ElectronService,private router: Router) {  
    this.initMenu();
  }

  ngOnInit() {

  }

    private initMenu() {
      let menu = this._electronService.remote.Menu.getApplicationMenu();

      let pageMenu  = new this._electronService.remote.MenuItem(
        {
          label: 'Page',
              submenu: [
                  { label: 'Page 1', click: () => this.router.navigate(['/page1']) },
                  { label: 'Page 2', click: () => this.router.navigate(['/page']) },
              ],
        }
      );

      menu.insert(1, pageMenu);

      this._electronService.remote.Menu.setApplicationMenu(menu);

    }
}

Make Sure the pages exists on routing file, otherwise will throw many route errors.

More details check Wonderfull Lib from ThorstenHans here

pamtbaau commented 6 years ago

@ameagol I'm not using Angular's routing, but the above mentioned workaround works fine for the original issue. Maybe the workaround might also work in your use case, which might make the extra library a bit superfluous for an issue that will be properly fixed in due time.

I use menus in the following way:

...
import { remote } from 'electron';
const { Menu } = remote;

@Component({
    selector: 'main-component',
    templateUrl: 'main.component.html',
})

export class MainComponent implements OnDestroy {
    private menu: Electron.Menu;
    ...
    constructor() {
        this.initMenu();
    }

    public onMenu() {
        this.menu.popup();
    }

    private initMenu() {
        const template: Electron.MenuItemConstructorOptions[] = [
            {
                label: 'Set exchange rates',
                click: () => this.zone.run(() => this.showExchangeRateDialog = true),
            },
            {
                label: 'Set stock prices',
                click: () => this.zone.run(() => this.showStockPriceDialog = true),
            },
        ];

        this.menu = Menu.buildFromTemplate(template);
    }
}
pamtbaau commented 6 years ago

@JiaLiPassion Great news! Thanks for this zone fixup for Electron. Would love to test this out in my application. When will it be released and how should I use it?

JiaLiPassion commented 6 years ago

@pamtbaau, thank you, I have created a sample repo to describe how to use, https://github.com/JiaLiPassion/zone-electron, please use the new release to test your application.

pamtbaau commented 6 years ago

Maybe I have a wrong perception as to how this feature solves the electron zone issues.

The next piece of code is a declaration of a menuitem using this.zone.run() as workaround:

const template: MenuItemConstructorOptions[] = [
    {
        label: `Delete ${hero.name}`,
        click: () => this.zone.run(() => this.deleteHero(hero)),
    },
];

I hoped I could now simply remove the this.zone.run() wrapper:

const template: MenuItemConstructorOptions[] = [
    {
        label: `Delete ${hero.name}`,
        click: () => this.deleteHero(hero),
    },
];

But no luck... Without the this.zone.run(), changeDetection doesn't work. Could you please give me some hints as to how a menuitem should be declared right now?

JiaLiPassion commented 6 years ago

@pamtbaau , I have tested the menuItem.click(), it works, so could you check here, https://github.com/JiaLiPassion/zone-electron/blob/master/src/app/components/home/home.component.ts#L108

and make sure

  1. you updated to zone.js 0.8.19
  2. in your src/polyfill.ts, add import 'zone.js/dist/zone-patch-electron'.
pamtbaau commented 6 years ago

@JiaLiPassion I have tested your suggestions, but it's not working

  1. Downloaded your app: It works
    Noticed that you are using package git+https://github.com/JiaLiPassion/zone.js.git#electrondist.
    NB. When installing your package, npm returns: +zone.js 0.8.18
  2. npm install --save zone.js (yields 0.8.19)
    You app doesn't work
  3. npm install --save git+https://github.com/JiaLiPassion/zone.js.git#electrondist
    You app works...

Same with my own app. It only works when installing git+https://github.com/JiaLiPassion/zone.js.git#electrondist

JiaLiPassion commented 6 years ago

@pamtbaau , thank you for your detail information, it seems 0.8.19 not release the newest contents, I will check it, please use git+https://github.com/JiaLiPassion/zone.js.git#electrondist for test now.

JiaLiPassion commented 6 years ago

@pamtbaau , I have checked 0.8.19 and found I made a typo, I will fix it and please use my branch for test by now.