isaacplmann / ngx-tour

Product Tour Built in Angular
https://isaacplmann.github.io/ngx-tour
MIT License
246 stars 100 forks source link

ngx-tour-ng-bootstrap broken with es6 #77

Closed dhalperi closed 6 years ago

dhalperi commented 6 years ago

Hey,

I'm finding that ngx-tour does not work for Angular projects set to target es6. In these cases, Angular apparently fails to inject the Router when constructing the NgbTourService, leading to this trace:

ng:///AppModule/AppComponent_Host.ngfactory.js:5 ERROR TypeError: Cannot read property 'events' of undefined
    at NgbTourService.TourService.startAt (webpack-internal:///../../../../ngx-tour-ng-bootstrap/ngx-tour-ng-bootstrap.js:97)
    at NgbTourService.TourService.start (webpack-internal:///../../../../ngx-tour-ng-bootstrap/ngx-tour-ng-bootstrap.js:86)
    at AppComponent.ngOnInit (webpack-internal:///../../../../../src/app/app.component.ts:34)
    at checkAndUpdateDirectiveInline (webpack-internal:///../../../core/esm2015/core.js:10507)
    at checkAndUpdateNodeInline (webpack-internal:///../../../core/esm2015/core.js:12030)
    at checkAndUpdateNode (webpack-internal:///../../../core/esm2015/core.js:11973)
    at debugCheckAndUpdateNode (webpack-internal:///../../../core/esm2015/core.js:12852)
    at debugCheckDirectivesFn (webpack-internal:///../../../core/esm2015/core.js:12797)
    at Object.eval [as updateDirectives] (ng:///AppModule/AppComponent_Host.ngfactory.js:9)
    at Object.debugUpdateDirectives [as updateDirectives] (webpack-internal:///../../../core/esm2015/core.js:12786)

Deeper reproduction instructions below.

  1. Is this a known issue?
  2. Is this expected, or a bug?

es6 is pretty nice, and Angular has supported it since September -- ng-cli version v1.5.0.

Thanks!


This repo https://github.com/dhalperi/ngx-tour-bug has three commits:

  1. Create a stock Angular project ng-cli new testtour.
  2. Add the simplest possible version of ngx-tour-ng-bootstrap (https://github.com/dhalperi/ngx-tour-bug/commit/5e3451ce02e85c162392cab17e42e27db0b18e91).
  3. Swap the target from es5 to es6 (https://github.com/dhalperi/ngx-tour-bug/commit/2f44c1d6a5e2ebee58476a0136495434e5245cca).

After the second commit, ngx-tour-ng-bootstrap seems to work great.

After switching to es6, the library fails as described above -- see the developer console error messages.

isaacplmann commented 6 years ago

Thanks for the thorough bug report. It's very nice to have a simple repo that reproduces the problem.

I submitted a PR to your repo that switch to using the ngx-tour-md-menu version - which apparently works in es6 mode. I really don't know why the ng-bootstrap version would not work in es6 mode. At first I thought it might be the underlying ng-bootstrap library itself, but the error is happening in the ngx-tour code.

And as I was typing up the response I had another idea.

This workaround will work. I'm not sure why it's necessary only for es6 though.

import { BrowserModule } from '@angular/platform-browser';
import { NgModule } from '@angular/core';
import { TourNgBootstrapModule, TourService as NgbTourService } from 'ngx-tour-ng-bootstrap';
import { TourModule, TourService } from 'ngx-tour-core';
import { RouterModule } from '@angular/router';

import { AppComponent } from './app.component';

const routing = RouterModule.forRoot([]);

@NgModule({
  declarations: [
    AppComponent
  ],
  imports: [
    BrowserModule,
    routing,
    TourNgBootstrapModule.forRoot(),
    TourModule.forRoot(),
  ],
  providers: [
    { provide: NgbTourService, useExisting: TourService },
  ],
  bootstrap: [AppComponent]
})
export class AppModule { }
dhalperi commented 6 years ago

Hey guess what, the bug is in Angular.

Specifically, Angular only supports ES6 properly in AOT mode which is used neither by default (ng serve defaults to JIT, to enable eot add --aot) nor is even possible to use for tests.

I think this means we have to target ES5 until Angular either stops using JIT for tests or supports ES6 in JIT mode.

My gut feeling now is that we've been getting lucky so far that our tests haven't been broken by our use of ES6. The repro here happens to be one case where one of the known ES6+JIT bugs crops up.

So we do have to go make our code target ES5 if we want our tests to reliably pass, even though we can use ES6 in our prod builds (which use AOT).

dhalperi commented 6 years ago

Thanks @isaacplmann for digging and for the workaround -- I think that since we've just been getting lucky so far, we'll port back to es5 where I am optimistic the bug won't show up.