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

Support rxjs operators w/ different import syntaxes #73

Closed
davidenke opened this issue Jul 14, 2017 · 14 comments · Fixed by #82
Closed

Support rxjs operators w/ different import syntaxes #73

davidenke opened this issue Jul 14, 2017 · 14 comments · Fixed by #82
Labels

Comments

@davidenke
Copy link
Contributor

Building with rxjs version 5.4.2 throws an error.
Started build with ng-packagr -p ./ng-package.json.

Console output (shortened, removed usual npm error message:

Building Angular library from ./ng-package.json

BUILD ERROR
'merge' is not exported by node_modules/rxjs/observable/merge.js
Error: 'merge' is not exported by node_modules/rxjs/observable/merge.js
...

In the moment I have no idea why this happens, since Observable.merge hasn't been removed and is still exported properly at first sight...

@davidenke davidenke changed the title RxJS 5.4.2 issues build error with rxjs@5.4.2 Jul 14, 2017
@dherges
Copy link
Contributor

dherges commented Jul 14, 2017

@davidenke can you reproduce this?

I think I've updated a few of "our's" libraries today to latest versions of Angular and RxJS and they still build fine. Our CI's went through all fine and I think it went up to typescript 2.4 and rxjs 5.4.x - there was a build error with rxjs and typescript 2.4 which was resolved in the latest rxjs version?!?

@davidenke
Copy link
Contributor Author

@dherges you're right, it turned out that I had issues with lodash. Btw. have you ever experienced issues with building relating lodash imports?

@davidenke
Copy link
Contributor Author

davidenke commented Jul 19, 2017

Okay, I discovered this issue again and built a simple test case.
Presuppose the following file structure:

.
+-- ng-package.json
+-- package.json
+-- public_api.ts
+-- tsconfig.json

Content of ng-package.json:

{
  "$schema": "./node_modules/ng-packagr/ng-package.schema.json",
  "src": ".",
  "workingDirectory": "./.packagr-cache",
  "lib": {
    "entryFile": "./public_api.ts",
    "externals": {}
  }
}

Content of package.json:

{
  "name": "@test/test",
  "version": "0.0.0",
  "description": "",
  "scripts": {
    "build": "ng-packagr -p ./ng-package.json"
  },
  "peerDependencies": {
    "@angular/animations": "^4.0.0",
    "@angular/cdk": "2.0.0-beta.8",
    "@angular/common": "^4.0.0",
    "@angular/core": "^4.0.0",
    "@angular/material": "2.0.0-beta.8",
    "rxjs": "^5.4.2"
  },
  "devDependencies": {
    "@angular/animations": "^4.0.0",
    "@angular/cdk": "2.0.0-beta.8",
    "@angular/common": "^4.0.0",
    "@angular/compiler": "^4.0.0",
    "@angular/core": "^4.0.0",
    "@angular/forms": "^4.0.0",
    "@angular/http": "^4.0.0",
    "@angular/material": "2.0.0-beta.8",
    "@angular/platform-browser": "^4.0.0",
    "@compodoc/compodoc": "^1.0.0-beta.10",
    "codelyzer": "^3.1.2",
    "http-server": "^0.10.0",
    "ng-packagr": "^1.0.0-pre.10",
    "rxjs": "^5.4.2",
    "sass-lint": "^1.10.2",
    "tslint": "^5.5.0",
    "tslint-microsoft-contrib": "^5.0.1",
    "zone.js": "^0.8.14"
  }
}

Content of public_api.ts:

import { CommonModule } from '@angular/common';
import { NgModule } from '@angular/core';
import { LIVE_ANNOUNCER_PROVIDER, MdButtonModule, MdCommonModule, MdIconModule, OverlayModule, PortalModule } from '@angular/material';

@NgModule({
  imports: [
    OverlayModule,
    PortalModule,
    CommonModule,
    MdButtonModule,
    MdCommonModule,
    MdIconModule
  ],
  exports: [
    OverlayModule,
    PortalModule,
    CommonModule,
    MdButtonModule,
    MdCommonModule,
    MdIconModule
  ],
  declarations: [],
  providers: []
})
export class TestModule {
}

Content of tsconfig.json

{
  "compileOnSave": false,
  "compilerOptions": {
    "outDir": "./dist/out-tsc",
    "baseUrl": "./src",
    "sourceMap": true,
    "declaration": false,
    "module": "es2015",
    "moduleResolution": "node",
    "emitDecoratorMetadata": true,
    "experimentalDecorators": true,
    "target": "es2015",
    "typeRoots": [
      "node_modules/@types"
    ],
    "types": [],
    "lib": [
      "es2015",
      "dom"
    ],
    "paths": {
      "@test/test": [
        "./public_api.ts"
      ]
    },
    "include": [
      "**/*.ts"
    ],
    "exclude": [
      "**/*.d.ts",
      "./.packagr-cache",
      "./src/test.ts",
      "./src/**/*.spec.ts"
    ]
  }
}

Install dependencies with yarn and build using the script: npm run build.

@davidenke davidenke reopened this Jul 19, 2017
@davidenke
Copy link
Contributor Author

After digging into it further, I think it is related to #62.

@DDeis
Copy link
Contributor

DDeis commented Jul 20, 2017

I'm getting a similar issue:

➜ DEBUG=true node_modules/.bin/ng-packagr -p ng-package.json
Building Angular library from ng-package.json
...
[debug] rollup /home/ahmad/Documents/Damien/Projets/ng-formly/.ng_build/ts//ng-formly.js to /home/ahmad/Documents/Damien/Projets/ng-formly/.ng_build/ng-formly/ng-formly.js (es)

BUILD ERROR
'$$observable' is not exported by node_modules/rxjs/symbol/observable.js
Error: '$$observable' is not exported by node_modules/rxjs/symbol/observable.js
    at error (/home/ahmad/Documents/Damien/Projets/ng-formly/node_modules/rollup/dist/rollup.js:177:12)
    ...

Tried to add "rxjs/symbol/observable": "Rx.Symbol.observable" to externals in ng-packaged but it didn't worked.

@davidenke
Copy link
Contributor Author

@DDeis in my case the declaration of externals used by material helped:

{
  "$schema": "./node_modules/ng-packagr/ng-package.schema.json",
  "lib": {
    "entryFile": "./public_api.ts",
    "externals": {
      "rxjs/observable/forkJoin": "Rx.Observable",
      "rxjs/observable/fromEvent": "Rx.Observable",
      "rxjs/observable/merge": "Rx.Observable",
      "rxjs/observable/of": "Rx.Observable",
      "rxjs/observable/throw": "Rx.Observable",
      "rxjs/operator/auditTime": "Rx.Observable.prototype",
      "rxjs/operator/catch": "Rx.Observable.prototype",
      "rxjs/operator/debounceTime": "Rx.Observable.prototype",
      "rxjs/operator/do": "Rx.Observable.prototype",
      "rxjs/operator/filter": "Rx.Observable.prototype",
      "rxjs/operator/finally": "Rx.Observable.prototype",
      "rxjs/operator/first": "Rx.Observable.prototype",
      "rxjs/operator/map": "Rx.Observable.prototype",
      "rxjs/operator/share": "Rx.Observable.prototype",
      "rxjs/operator/startWith": "Rx.Observable.prototype",
      "rxjs/operator/switchMap": "Rx.Observable.prototype",
      "rxjs/operator/takeUntil": "Rx.Observable.prototype"
    }
  }
}

@dherges Would it make sense to add those import usages to the external defaults as well?

@dherges
Copy link
Contributor

dherges commented Jul 20, 2017

Yes sure!

Personally, I prefer the import 'rxjs/add/* syntax for operators. There should be an integration sample for both ways so we make sure it works in both the "add" syntax and the "direct import".

If included, it would be nice to mirror the whole list

@davidenke
Copy link
Contributor Author

@dherges me too. Shall I pr that, though?

@DDeis
Copy link
Contributor

DDeis commented Jul 20, 2017

My bad, for me adding "rxjs/symbol/observable": "Rx.Symbol" to externals also works, I wasn't testing with the good ng-package previously...

I ended up with:

{
  "$schema": "./node_modules/ng-packagr/ng-package.schema.json",
  "lib": {
    "entryFile": "src/index.ts",
    "externals": {
      "rxjs/observable/fromPromise": "Rx.Observable",
      "rxjs/operator/debounceTime": "Rx.Observable.prototype",
      "rxjs/operator/map": "Rx.Observable.prototype",
      "rxjs/operator/toPromise": "Rx.Observable.prototype",
      "rxjs/symbol/observable": "Rx.Symbol"
    }
  }
}

Though I also made it works by adding rollup-plugin-commonjs to ng-packagr with the commonjs config:

{
   include: 'node_modules/rxjs/**' // also works with 'node_modules/**'
}

This way, we don't need RxJS dependencies in ROLLUP_GLOBALS nor in externals so it seems to me that it's less tedious. Also with externals I get a warning '$$observable' is imported from external module 'rxjs/symbol/observable' but never used that I don't get with rollup-plugin-commonjs.

@davidenke
Copy link
Contributor Author

davidenke commented Jul 20, 2017

I agree, that looks cleaner. But I have no idea if including all node_modules/* equally has any side effects, so I'd stick to include: 'node_modules/rxjs/**'.

@DDeis
Copy link
Contributor

DDeis commented Jul 21, 2017

You may be right, I edited the PR to only include 'node_modules/rxjs/**' but allowing to add other dependencies manually (for #62).

@dherges
Copy link
Contributor

dherges commented Jul 22, 2017

tbh, I cannot fully undertand how this issue arrives and why the change is needed.

In dherges/ng-packaged, usage of rxjs 5.4.2 is demonstrated.

There's a service using Observables and operators w/ the maybe "old-fashioned" (?) import rxjs/add/operator syntax.

If this about the "newer" import 'rxjs/operator/map', ok. In #57, this was addressed. The relevant change is demonstrated in one of the integration samples

I am open to PRs that improve ng-packagr. Right now, I consider this issue a "support request" or "help wanted". To met, it does not look like a bug or defect in here...

@davidenke
Copy link
Contributor Author

Because Angular Material uses the "ugly" import style. We can leave a hint in the readme what to import, or just add the rxjs dependencies in the defaults.

@dherges dherges changed the title build error with rxjs@5.4.2 Support rxjs operators w/ different import syntaxes Jul 27, 2017
@github-actions
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

This action has been performed automatically by a bot.

@github-actions github-actions bot locked and limited conversation to collaborators Jun 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging a pull request may close this issue.

3 participants