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

module is not compatible with rollup.js bundling and tree-shaking. #48

Closed
webprofusion-chrisc opened this issue Oct 13, 2016 · 30 comments
Closed
Assignees

Comments

@webprofusion-chrisc
Copy link

webprofusion-chrisc commented Oct 13, 2016

The current sample code does not cover the inclusion of the module/component via @NgModule app import.

This causes a problem for me as trying to import the ChartistModule (rather than importing the component at a page level as it used to be) results in the error Unexpected value 'ChartistComponent' exported by the module 'ChartistModule' , suggesting that I'm importing it incorrectly.

@webprofusion-chrisc
Copy link
Author

Should also say that I'm using Ionic 2, was working fine before Angular 2 moved to modules.

@ghost
Copy link

ghost commented Oct 13, 2016

In Ionic2 RC0, you need to make some changes in the rollup config for angular2-chartist to work:

In node_modules@ionic\app-scripts\config\rolllup.config.js, change the commonjs() line to :
commonjs({ include:[ 'node_modules/angular2-chartist/**', 'node_modules/rxjs/**', 'node_modules/chartist/**' ], namedExports:{ 'node_modules/angular2-chartist/dist/angular2-chartist.js' : ['ChartistComponent'] } })

You can see reference to this issue here

@willsoto
Copy link
Owner

Is there anything I can do here to avoid extra configuration? Or is this
just an ionic quirk?

On Thu, Oct 13, 2016, 12:52 PM Maxence Lehuraux [email protected]
wrote:

In Ionic2 RC0, you need to make some changes in the rollup config for
angular2-chartist to work:

In node_modules@ionic https://github.com/ionic\app-scripts\config\rolllup.config.js,
change the commonjs() line to :
commonjs({
include:[
'node_modules/angular2-chartist/',
'node_modules/rxjs/
',
'node_modules/chartist/**'
],
namedExports:{
'node_modules/angular2-chartist/dist/angular2-chartist.js' :
['ChartistComponent']
}
})

You can see reference to this issue here
ionic-team/ionic-app-scripts#68


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#48 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACqZMiD4SgYNj29-Q6NxYKlxczIoMp8gks5qzowZgaJpZM4KVbh2
.

@ghost
Copy link

ghost commented Oct 14, 2016

It can be fixed on your side from what I read. It seems to be more of an incompatibility with how your component is distributed and how rollup would like it to be distributed.

Here's what @danbucholtz says in valor-software/ng2-charts#440:

We're going to provide some instructions to Angular library creators (and really any library creators) on how to distribute their dist directory with commonjs modules AND es2015 modules and link them up in the package.json using the main and module fields.
Once this library follows the Angular convention, everything will just work.

@webprofusion-chrisc
Copy link
Author

Thanks, yes it also looks like they're going to default to webpack soon instead of rollup. Renaming issue to reflect rollup incompatibility.

@webprofusion-chrisc webprofusion-chrisc changed the title Example code isn't updated to Angular 2.0 (NgModule) module is not compatible with rollup.js bundling and tree-shaking. Oct 16, 2016
@willsoto willsoto self-assigned this Oct 16, 2016
@webprofusion-chrisc
Copy link
Author

So my final fix/workaround for this was to install the latest @types/chartist and use a custom rollup config with the following namedExport for angular2-chartist:

commonjs({
            namedExports: {
                // chartist
                'node_modules/angular2-chartist/dist/angular2-chartist.js': ['ChartistComponent'],
                ...
            }
        }),

As an aside, I did get a weird problem where my line chart had a filled black area, even with showArea set to false, worked around with with css. Assume this is more a problem/change with base chartist library.

@willsoto
Copy link
Owner

I'm currently working on a fix for this to hopefully avoid any extra
configuration. For now, you can use workarounds obviously. I'll mention
something here once the fix is released.

On Tue, Oct 18, 2016 at 12:21 AM Christopher Cook [email protected]
wrote:

So my final fix/workaround for this was to install the latest
@types/chartist and use a custom rollup config with the following
namedExport for angular2-chartist:

commonjs({
namedExports: {
// chartist
'node_modules/angular2-chartist/dist/angular2-chartist.js': ['ChartistComponent'],
...
}
}),

As an aside, I did get a weird problem where my line chart had a filled
black area, even with showArea set to false, worked around with with css.
Assume this is more a problem/change with base chartist library.


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#48 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACqZMj32fvFvdKYoRnyAF1EwWeU3LDWSks5q1ElCgaJpZM4KVbh2
.

@danbucholtz
Copy link

Thanks @Paradox41!

Thanks,
Dan

@willsoto
Copy link
Owner

I released this as 0.10.0. Hopefully there are no breaking changes, please let me know if this fixes the problems people are seeing and if the extra configuration can be removed.

@webprofusion-chrisc
Copy link
Author

Thanks, it might be closer but it's not working for me, unless I'm using it wrong? I'm assuming I just import the Module and the rest of my app can then use <x-chartist></x-chartist> as normal. this would be the same for any angular 2.0 app, not just Ionic based ones.

`import { ChartistModule } from 'angular2-chartist';`
...
imports: [
    IonicModule.forRoot(MyApp, { mode: 'ios' }),
    ChartistModule
  ],
...

Error message:

[09:39:23]  typescript: C:/Work/SVN/Dashboard/app/dashboard/src/app/app.module.ts, line: 42                                                                                                   
            Module '"C:/Work/SVN/Dashboard/app/dashboard/node_modules/angular2-chartist/dist/chartist.component"'                                                                             
            has no exported member 'ChartistModule'.                                                                                                                                          

      L41:  import { AdminOptionsSuperuser } from './../components/admin-options-superuser/admin-op                                                                                           
      L42:  import { ChartistM                                                                                                                                  
dule } from 'angular2-chartist';      

Any chance you could use the quick ionic 2 starter (http://ionicframework.com/docs/v2/getting-started/installation/) to try to display a chart for yourself? I appreciate that might take time but I also note that the example code in the readme is based on an old angular 2 rc pre-NgModule and that could be a stumbling block for people trying to use this component/module. perhaps solving this would help resolve that too.

@willsoto
Copy link
Owner

I wonder if this has to do with Typings. Currently I can't specify more than one typings location (or concat typings). Looking at the path to the file node_modules/angular2-chartist/dist/chartist.component"', I would expect to see node_modules/angular2-chartist/dist/angular2-chartist.js.

What version are you on?

@webprofusion-chrisc
Copy link
Author

I've tried angular2-chartist 0.9.5 and 0.10.0. No old typings or typings.json, just using npm types for chartist.
Ionic 2.0 RC1, with Angular 2.0.0

I've given up for now as I've spent an impressive number of hours trying to get chartist working in some form and the people I'm doing the work for won't be impressed :)

Can you confirm that this currently works in regular Angular 2.0.x?

@willsoto
Copy link
Owner

Yes, I can run it locally with Angular 2. I'll have to spend more time
tonight with it. In the meantime, does it work with the above workaround?

On Wed, Oct 19, 2016, 9:35 AM Christopher Cook [email protected]
wrote:

I've tried angular2-chartist 0.9.5 and 0.10.0. No old typings or
typings.json, just using npm types for chartist.
Ionic 2.0 RC1, with Angular 2.0.0

I've given up for now as I've spent an impressive number of hours trying
to get chartist working in some form and the people I'm doing the work for
won't be impressed :)

Can you confirm that this currently works in regular Angular 2.0.x?


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#48 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACqZMvpEjXlu0_uP5h-ksqOOGWb0Gkmsks5q1hycgaJpZM4KVbh2
.

@webprofusion-chrisc
Copy link
Author

Unfortunately I got 0.9.5 working, then saw your update and applied it. Should have committed my changes! Now can't seem to get back to a working version despite trying all sorts of variations. Have currently reverted to using Chartist directly in my own component but the Ionic component lifecycle is causing issues when page navigation takes place, which is something your component seemed to handle fine (which is why I'd eventually like to get it working again).

@willsoto
Copy link
Owner

I'll do my best to fix it tonight. Sorry for the trouble.

On Wed, Oct 19, 2016 at 9:48 AM Christopher Cook [email protected]
wrote:

Unfortunately I got 0.9.5 working, then saw your update and applied it.
Should have committed my changes! Now can't seem to get back to a working
version despite trying all sorts of variations. Have currently reverted to
using Chartist directly in my own component but the Ionic component
lifecycle is causing issues when page navigation takes place, which is
something your component seemed to handle fine (which is why I'd eventually
like to get it working again).


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#48 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACqZMg7X2rs2Akyiu-6NPC84k6-RJgXmks5q1h-6gaJpZM4KVbh2
.

@webprofusion-chrisc
Copy link
Author

No worries, good luck!

willsoto added a commit that referenced this issue Oct 19, 2016
@willsoto
Copy link
Owner

willsoto commented Oct 19, 2016

@webprofusion-chrisc can you do me a favor? Go ahead and npm install the master branch from here, cd into it and run npm i && npm run build:dist (I think this might be needed since I don't keep the dist in version control) and let me know what happens for you. This won't work I just realized, need more files to build. You could use npm link if you wanna clone the repo.

I just tested again locally and everything works as expected, the module is being exported correctly. I refactored a bit so it is one file now, but that shouldn't change anything for consumers to be honest.

If you continue to have issues, it could be an ionic quirk that I have to look into more.

Thanks.

@willsoto
Copy link
Owner

@webprofusion-chrisc update on this. I did as you asked and tested with the ionic 2 starter using the updated build, everything worked as expected. I get some .d.ts errors that are unrelated to this project though, but the module is correctly located and importable.

thing

I am gonna go ahead and cut a release for this.

@ghost
Copy link

ghost commented Oct 19, 2016

I just tested and I still had to modify the rollup.config.js to have this in commonjs():

{ namedExports:{ 'node_modules/angular2-chartist/dist/angular2-chartist.js' : ['ChartistModule'] } }

Otherwise, still works good

@webprofusion-chrisc
Copy link
Author

Hmm, when I tried it ionic build browser --release (which is quite a strict compile) I get:

[20:15:33]  Error: Unexpected value 'ChartistModule' imported by the module 'AppModule'                                                                                              
[20:15:33]  ngc failed                                                                                                                                                               
[20:15:33]  ionic-app-script task: "build"                                                                                                                                           
[20:15:33]  Error: Error       

If I just do ionic serve I get:

[20:22:54]  transpile update started ...                                                                                                                                             
[20:22:57]  transpile update finished in 2.52 s                                                                                                                                      
[20:22:57]  bundle update started ...                                                                                                                                                
[20:23:02]  bundle update failed: 'ChartistModule' is not exported by node_modules\angular2-chartist\dist\angular2-chartist.js                                                       
            (imported by src\app\app.module.ts). For help fixing this error see https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module                 
[20:23:02]  watch ready 

That's with or without a custom rollup config entry. Not sure whats different.
It's obviously rollup that causing the issue, so when ionic switch to webpack shortly it will hopefully resolve itself.

@willsoto
Copy link
Owner

Unfortunately, I am not sure what is happening. I was able to successfully compile an ionic app using the module. Is it possible there is some configuration you have somewhere that is throwing it off?

@webprofusion-chrisc
Copy link
Author

Yes, it's possible! This is a different machine (and app) from the one I had problems with though, so I'll try again on the other machine tomorrow.

@webprofusion-chrisc
Copy link
Author

Alas I just can't get it working. Sorry, just one more check: can you (or anyone else) confirm that ionic build browser --release builds OK. As I understand it ngc (compilation) doesn't run using the standard ionic serve, just ionic build

@webprofusion-chrisc
Copy link
Author

Ok, I've narrowed it down -ionic serve works (using the current git version of the ionic app scripts) but a full build withionic build doesn't.

For angulars AOT compilation to work the package needs a .metadata.json (alongside the .d.ts) generated by the angular-cli command ngc (equivalent to typescript tsc but should generate a .metadata.json file). Unfortunately although I can get it to compile, no .metadata.json file appears, so I'm still doing something wrong.

@ghost
Copy link

ghost commented Oct 21, 2016

I think ionic build is deprecated.
Use "npm run build" or "npm run build --dev" instead which will use the ionic-app-scripts to build your application.
More info here: ionic-app-scripts

@webprofusion-chrisc
Copy link
Author

@mlehuraux-maf ionic build isn't deprecated at all, it runs the same build step (ionic-app-scripts). The remaining issue is compatibility with angular 2 AOT step using ngc, although you have indicated you can get it to build. I can assure you that the process is not working for me, it does work with npm run build --dev but I'm guessing that skips AOT.

@jensstruemper
Copy link
Contributor

I'm trying to reproduce the findings here. I've setup two identical Ionic projects with a minimal chartist setup one with the current apps-scripts using rollup and one with the beta apps-scripts using webpack.

Rollup:
Regardless of ionic serve or ionic run android I see bundle failed: 'ChartistModule' is not exported.. This is a surprise as I think it should work without AoT compilation?! Adding the custom rollup config didn't change anything.
Webpack:
Using webpack the ionic serve works fine and I can see the chart being displayed. Butnpm run build / ionic run android still issues the "Chartist Module is not exported" error.

Anything screwed up in my simplified setup?`

Versions:
ionic 2 Rc1
[email protected]
[email protected]
@types/[email protected]

home.ts

import { Component } from '@angular/core';
import { NavController } from 'ionic-angular';
import { ChartType } from 'angular2-chartist';

@Component({
  selector: 'page-home',
  template: `
    <x-chartist
      [data]="data"
      [type]="type">
    </x-chartist>
  `
})

export class HomePage {
 type: ChartType;
 data: any;

  constructor(public nav:NavController) {
    this.type = 'Line';
    this.data = {
      "labels": [
        "Jan",
        ......
        "Dec"
      ],
      "series": [
        [5, 4, 3, 7, 5, 10 ,3, 4, 8, 10 ,6, 8],
        [3, 2, 9, 5, 4, 6, 4, 6, 7, 8, 7, 4]
      ]
    }
  }
}

app.modules.ts


import { NgModule } from '@angular/core';
import { IonicApp, IonicModule } from 'ionic-angular';
import { MyApp } from './app.component';
import { AboutPage } from '../pages/about/about';
import { ContactPage } from '../pages/contact/contact';
import { HomePage } from '../pages/home/home';
import { TabsPage } from '../pages/tabs/tabs';
import { ChartistModule } from 'angular2-chartist';

@NgModule({
  declarations: [
    MyApp,
    AboutPage,
    ContactPage,
    HomePage,
    TabsPage
  ],
  imports: [
    ChartistModule,
    IonicModule.forRoot(MyApp)
  ],
  bootstrap: [IonicApp],
  entryComponents: [
    MyApp,
    AboutPage,
    ContactPage,
    HomePage,
    TabsPage
  ],
  providers: []
})
export class AppModule {}

@webprofusion-chrisc
Copy link
Author

@jensstruemper while 'ionic serve' (with Rollup) doesn't run the ngc compiler step, it does bundle with Rollup which needs the type declarations in order to identify which exports can be pruned from the output. You might be able to fix that by adding a reference to the chartist module in the declarations.d.ts.

I'm currently using the dev webpack version of the build scripts (with build --dev specified in the package.json build step). Attempting to build in production (so with AOT being applied and ngc being used instead of tsc) also results in the Module Not Exported error. I believe this is because AOT requires libraries to have a .metdata.json file alongside their .d.ts file, although my attempts to generate that myself didn't work.

@jensstruemper
Copy link
Contributor

jensstruemper commented Oct 27, 2016

@webprofusion-chrisc thanks for sharing some background on the ionic run / build process! I added declare module 'angular2-chartist';to the declarations.d.ts file but it results in an error: Cannot find name 'ChartType'.

On webpack: If I add the "--dev" argument ionic run android would stall at "build dev finished" so no apk is generated.

Would be happy to continue with JiT for now but as it appears I can't even get the most basic setup running at the moment.

@jensstruemper
Copy link
Contributor

With the introduction of app-scripts 0.39 I can use "--dev" argument now (no more freezing) so it is working with JiT. I tried to generate the metadata.json using ngc too. Although the compilation finished successfully no metadata.json was generated. Hope the docs for angular2 module creators / maintainers get better soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants