CaliStyle / ng-intercom

Angular 7+ Wrapper for Intercom.com
MIT License
54 stars 46 forks source link

Function calls are not supported in decorators but 'IntercomModule' was called. - Regression in beta.6 #74

Closed chrishawn closed 5 years ago

chrishawn commented 5 years ago

_Originally posted by @chrishawn in https://github.com/CaliStyle/ng-intercom/issue_comments#issuecomment-430313444_

developcodeza-matthew commented 5 years ago

I'm having the same issue.

It fails only when building it for prod with ng build --prod (with AOT enabled).

It would seem that when using the @NgModule decorator, the module will be statically analyzed by the AOT compiler (if AOT is set of course).

If this may explain the issue, here is a StackOverflow post outlining what can be done.

Here are my dependancies.

"@agm/core": "^1.0.0-beta.3",
"@angular/animations": "^6.1.4",
"@angular/cdk": "^6.4.6",
"@angular/common": "^6.1.4",
"@angular/compiler": "^6.1.4",
"@angular/core": "^6.1.4",
"@angular/forms": "^6.1.4",
"@angular/http": "^6.1.4",
"@angular/material": "^6.4.6",
"@angular/platform-browser": "^6.1.4",
"@angular/platform-browser-dynamic": "^6.1.4",
"@angular/platform-server": "^6.1.4",
"@angular/router": "^6.1.4",
"@ng-bootstrap/ng-bootstrap": "^3.1.0",
"@ng-select/ng-select": "^2.5.1",
"@types/algoliasearch": "^3.27.6",
"@types/papaparse": "^4.5.2",
"algoliasearch": "^3.30.0",
"angularfire2": "^5.0.1",
"angulartics2": "^6.3.0",
"animate.css": "^3.7.0",
"bootstrap": "^4.1.3",
"classlist.js": "^1.1.20150312",
"core-js": "^2.5.7",
"file-saver": "^2.0.0-rc.2",
"firebase": "^5.5.1",
"font-awesome": "^4.7.0",
"hammerjs": "^2.0.8",
"install": "^0.12.1",
"intl": "^1.2.5",
"jquery": "^3.3.1",
"moment": "^2.22.2",
"ng-intercom": "^6.0.0-beta.6",
"ngx-mat-select-search": "^1.3.0",
"ngx-stripe": "^6.0.0-rc.0",
"npm": "^5.8.0",
"papaparse": "^4.6.0",
"rxjs": "^6.3.2",
"rxjs-compat": "^6.3.2",
"sinon": "^4.5.0",
"underscore": "^1.9.0",
"web-animations-js": "^2.3.1",
"zone.js": "^0.8.26"
wbhob commented 5 years ago

@scott-wyatt the issue could be related to treating IntercomConfig as an interface. It doesn't get compiled to JS, so it would make sense that it can't be injected. https://github.com/CaliStyle/ng-intercom/blob/9957d161280bf48d2305caee657fd282ddaf55cb/src/app/ng-intercom/types/intercom-config.ts#L1

Vaishak018 commented 5 years ago

I'm also having the same issue.

Any workaround for now ?

LiamLB commented 5 years ago

I'm also having the same issue.

Any workaround for now ?

I disabled AoT for now which is really not ideal. You can do so in the angular.json configuration file.

"configurations": {
            "production": {
              "optimization": true,
              "outputHashing": "all",
              "sourceMap": false,
              "extractCss": true,
              "namedChunks": false,
              "aot": false,
              "extractLicenses": true,
              "vendorChunk": false,
              "buildOptimizer": false,
wbhob commented 5 years ago

Sorry about the delay, folks. I'll test this on my end and let you know if this is the issue or if, as mentioned in the previous issue, it's ngpackagr.

wbhob commented 5 years ago

We're running this internally on Angular v7. Have you tried upgrading? I can't reproduce this issue.

wbhob commented 5 years ago

I made some changes on the ng-intercom/wilson-74 branch. If you're experiencing this issue, I'd invite you to pull this down, build locally, and install locally and see if it's fixed.

Alternatively, 7.0-beta.0 will be out soon (after approval by scott) if you want to stick to the npm version.

LiamLB commented 5 years ago

Thanks for the response. I'm unable to upgrade to Angular 7 at the moment and a manual install would be a bit painful for the pipeline. Just want to confirm that the issue was not reproducible with Angular 7 though?

Is there anything else possible? Will the 7.0-beta.0 release be backwards compatible with Angular 6? If so, will it possibly be released before the 6th of November (I have a major release around then)?

Sorry for the question bombardment.

LiamLB commented 5 years ago

Hi again, I've managed to upgrade to Angular 7. I'm still getting the error when building with AoT. I will try to build from your branch directly and let you know.

wbhob commented 5 years ago

If this is still an issue for you, we're open to PRs. At this point, CS can't throw a lot more resources at development, especially since we can't reproduce the issue. We will definitely continue to maintain by merging PRs, though

LiamLB commented 5 years ago

Just wanted to note this issue is not present in 6.0.0-beta.3 (npm install --save ng-intercom@6.0.0-beta.3). It is present in all the more recent versions.

eyalhakim commented 5 years ago

I'm having this issue as well, with Angular 7 and ng-intercom 6.0.0-beta.7.

From a research I did, it probably only happens given a certain tsconfig.json configuration.

wbhob commented 5 years ago

@eyalhakim please share your configuration

eyalhakim commented 5 years ago

I am using a Universal app.

tsconfig.json:

{ "compileOnSave": false, "compilerOptions": { "outDir": "./dist/out-tsc", "sourceMap": true, "declaration": false, "moduleResolution": "node", "emitDecoratorMetadata": true, "experimentalDecorators": true, "noUnusedLocals": true, "noUnusedParameters": true, "target": "es5", "downlevelIteration": true, "importHelpers": true, "typeRoots": [ "node_modules/@types" ], "lib": [ "es2017", "dom", "es2015.collection", "es2015.iterable" ] } }

tsconfig.app.json:

{ "extends": "../tsconfig.json", "compilerOptions": { "outDir": "../out-tsc/app", "baseUrl": "./", "module": "es2015", "types": [] }, "exclude": [ "test.ts", "**/*.spec.ts" ] }

wbhob commented 5 years ago

Can someone else confirm that changing your config to this works?

eyalhakim commented 5 years ago

@wbhob you mean, doesn't work right? aka causing the bug.

wbhob commented 5 years ago

Yes.

neilsoult commented 5 years ago

I am experiencing the bug as well, and have a similar config:

{
    "compileOnSave": false,
    "compilerOptions": {
        "allowSyntheticDefaultImports": true,
        "baseUrl": "./",
        "declaration": false,
        "emitDecoratorMetadata": true,
        "experimentalDecorators": true,
        "lib": [
            "dom",
            "esnext"
        ],
        "moduleResolution": "node",
        "noUnusedLocals": true,
        "outDir": "./dist",
        "sourceMap": true,
        "target": "es5"
    }
}

app:

{
    "extends": "../tsconfig.json",
    "compilerOptions": {
        "forceConsistentCasingInFileNames": false,
        "module": "es2015",
        "newLine": "lf",
        "noImplicitAny": true,
        "removeComments": true
    },
    "exclude": [
        "src/test.ts",
        "**/*.spec.ts"
    ]
}
wbhob commented 5 years ago

If you change the config, does it work @eyalhakim

neilsoult commented 5 years ago

@wbhob what is the configuration that you are using that does not produce the bug?

wbhob commented 5 years ago

Most importantly, it has commonjs as the module format which could be the issue. If that doesn't work, see what happens when the target is es6

junaidsarwarpk commented 5 years ago

@scott-wyatt issue still persists, previous commit did not help.

alexregier commented 5 years ago

Same here. Especially with the ng xi18n builder.

wbhob commented 5 years ago

People. We need a PR. We don't have the resources within the company to fix this now. We're looking for contributions from the community to fix this issue. We will review your PR if you submit one.

We can't reproduce this issue internally, either, and since that's the case, we don't need a fix for our own projects. If you want a fix, please contribute one.

Koslun commented 5 years ago

Also still seeing the same error as described as above when compiling with AOT. Using version 7.0.0-beta.0 with Angular 7. Using Angular CLI to build the project and using this TSConfig:

{
  "compileOnSave": false,
  "compilerOptions": {
    "outDir": "./dist/out-tsc",
    "sourceMap": true,
    "declaration": false,
    "moduleResolution": "node",
    "noUnusedLocals": true,
    "emitDecoratorMetadata": true,
    "experimentalDecorators": true,
    "strictPropertyInitialization": false,
    "target": "es5",
    "typeRoots": ["node_modules/@types"],
    "paths": { "stream": [ "../node_modules/readable-stream" ] },
    "lib": ["es2017", "dom"]
  }
}

The line "paths": { "stream": [ "../node_modules/readable-stream" ] }, is used to solve a specific issue with papaparse when compiling with Angular CLI.

This is currently blocking our upgrade to Angular 7 so will at least try to get a quick-fix to get it work with Angular 7, even if that means using a lower version.

Koslun commented 5 years ago

Looking at the code and comparing it to @angular/flex-layout, which has an optional static method, and ngx-cookie I only see two clear differences:

  1. I notice is that the value provided in this library is IntercomConfig which is an @Injectable provided later while in flex-layout there is a clearer distinction with the Injected token being created from the interface, see here. This injection token is further the only thing changed in the module file.
  2. A lot of components and directives are declared in the module, whereas in flex-layout only modules are imported and exported or the one service like in ngx-cookie.

So if it's due to a change in the module itself I figure the type of the injection token is the issue. Otherwise I think it's the build or other random difference like declaration of modules.

EDIT:

Forked this repository and tried adding that repository as an npm package to my own to try to iterate on a solution. But couldn't get IntercomModule imported into my workspace using the regular import from ng-intercom or diving further in. Build simply fails with ERROR in src/app/app.module.ts(22,32): error TS2307: Cannot find module 'ng-intercom'.

Finally copied the folder with the library into my own project and not only did the import start working, this error stopped happening. So that is at least one dirty quick-fix to the problem.

Find it more likely to be some strange build configuration now but think I'll try my proposed solution of changing how things are injected and see if that helps.

wbhob commented 5 years ago

If you want to make a PR, i'd be happy to take a look and merge it in. Thanks for investigating @Koslun

Koslun commented 5 years ago

@wbhob got it. I made the changes locally in a copy of the library I put into our project. It continued working but still couldn't build the project and try it out as it will appear on npm.

Will try to create a PR later this week depending on time. And then I'll see about testing it more or if you or someone else can do it.

wbhob commented 5 years ago

@Koslun if you run npm run build:dist, then cd into dist and run npm package, you can install the output .tgz file as an npm package to test! Hope this helps.

Koslun commented 5 years ago

@wbhob thanks will try this then, when I can get the time this week.

hassaanp commented 5 years ago

Hi folks, Is there any temporary hack or workaround for this?

wbhob commented 5 years ago

You could use a lower version @hassaanp or just build JiT

hassaanp commented 5 years ago

I am going ahead with JiT for now. Thanks @wbhob

Hesesses commented 5 years ago

Hello, thank you very much for creating this. Just installed this: npm install ng-intercom@latest --save -> "ng-intercom": "^7.0.0-beta.0", It works perfectly locally, but when creating --prod --aot build, getting the error also.

I'm using Angular 7.1

wbhob commented 5 years ago

Before you leave a comment:

We know this is an issue with AoT and Angular 7+. @Koslun is working on a fix. Don't leave another comment below unless you have something new to contribute or it will be marked off-topic.

Also, my previous comment:

People. We need a PR. We don't have the resources within the company to fix this now. We're looking for contributions from the community to fix this issue. We will review your PR if you submit one.

We can't reproduce this issue internally, either, and since that's the case, we don't need a fix for our own projects. If you want a fix, please contribute one.

Hesesses commented 5 years ago

@Koslun sorry to disturb you, but do you have any eta when the fix might be available? 🙏

lucagouty commented 5 years ago

@Koslun @wbhob I made a PR fixing the issue : https://github.com/CaliStyle/ng-intercom/pull/83

Also, as it was said before, npm install --save ng-intercom@6.0.0-beta.3 does fix the issue until this is merged

Koslun commented 5 years ago

@lucagouty Thanks for the fix, great find!

@Hesesses Reviewing it my proposed refactorization was different and would likely not have fixed the issue at hand. As it's been fixed now I think I'll avoid making any more needless changes.