ionic-team / ionic-app-scripts

App Build Scripts for Ionic Projects
http://ionicframework.com/
MIT License
608 stars 302 forks source link

Using CommonJS modules with rollup #16

Closed shlomiassaf closed 7 years ago

shlomiassaf commented 7 years ago

@adamdbradley I think you closed #3 in a rush.

angularfire2 and firebase are something you can't ignore when thinking ionic...

It's also not specific to them, it's a problem when using rollup, I think the rollup process needs to handle commonJS directories transparently.

I now have another, similar error with lodash

When I:

import { assign } from 'lodash';

I get:

[14:02:04]  bundle failed:  Error: Module /app/node_modules/lodash/lodash.js does not export assign (imported by /app/.tmp/data-source/some.service.js)

Again, there is a workaround but ionic developer shouldn't get hard time on simple tasks:

import assign from 'lodash/assign';

This is not perfect, there is a TS error:

[14:14:40]  src/data-source/feeding.service.ts(5,8): error TS1192: Module '"lodash/assign"' has no default export.

There's a switch in error between the 2, when TS is happy rollup isn't and vice versa.

BTW, rollup does a wired thing when doing:

import * as assign from 'lodash/assign';

assign will be the module, with a default property which is the assign function...

error_handler.js:45 EXCEPTION: Error in ./HomePage class HomePage - inline template:17:10 caused by: assign$2 is not a function

I know you considered #3 as someone else's problem but app-scripts is part of the ionic CLI and if the integration is not smooth you'll get ton's of complaints. If you integrate rollup you need it to be transparent.

danbucholtz commented 7 years ago

Hi @shlomiassaf,

We're not sure what you mean. We do support commonjs as we are using the latest version of the commonjs plugin.

https://github.com/driftyco/ionic-app-scripts/blob/master/config/rollup.config.js#L37

Please let me know if this fixes your issue. I'll re-open if needed.

Thanks, Dan

shlomiassaf commented 7 years ago

@danbucholtz Why did you close the issue? is this a common behaviour for people contributing to your project? You didn't solve it...

Now, before you run and close try to look into the issue.

I kindly ask you to try and integrate angularfire2 or lodash.

I'm trying to save you a lot of time and complaints later on, I'v spent a lot of hours on this beta, this is not how you should treat people who try to help.

This is insulting

Jus to clarify, the reason I react like that is that the same error was closed in #3 without a solution...

shlomiassaf commented 7 years ago

Here's another issue for you:

In watch mode when using skip property in the nodeModules plugin for rollup (within rollup.config.js) the skip is only valid for the first run.

Save something and the next update will error since using cache for rollup causes an issue. If you disable caching everything is fine, but you can't do that without changing the code of this lib.

I'v managed to workaround the angularfire issue and now this cache thing you introduced broke it...

I know sometime people pollute the issues but this is not the case, anyone who could use the beta.12 without docs should be taken seriously, its across 4 repos of integration.

danbucholtz commented 7 years ago

Hi,

I am seeing an issue here. I'll look into it. The goal is the user doesn't need to care if a 3rd party lib is commonjs or es2015 modules.

Thanks, Dan

shlomiassaf commented 7 years ago

I know it's the goal and I also now that you'll get tons of complaints for these 2 libs only...

I'm trying to help.

mlynch commented 7 years ago

@shlomiassaf we're looking into it and taking it seriously. This needs to Just Work or it won't be acceptable on our end. Apologies for the confusion.

shlomiassaf commented 7 years ago

@danbucholtz @mlynch This is really a core issue with the rollup bundling process. I can't import major libraries.

Now moment is giving me hard time, see this issue comment, I can run in dev but I can't build to a device.

BTW, I had to use moment since the Intl polyfil is not present so iOS safari can't run angular 2 if for example i'll use the date pipe.

danbucholtz commented 7 years ago

Hi @shlomiassaf,

We are aware of this issue and working on it. I don't have more to share right now but I hope to soon.

Thanks, Dan

danbucholtz commented 7 years ago

Hi @shlomiassaf,

I have successfully created a project with moment, lodash, node-uuid, localforage. All commonjs modules. You're right there is an issue here. I've come up with a solution. I'll have more details on it tomorrow.

In the meantime, the code changes to app-scripts to facilitate it are here: https://github.com/driftyco/ionic-app-scripts/pull/29

Thanks, Dan

shlomiassaf commented 7 years ago

@danbucholtz thanks!

I couldn't load moment via

import * as moment from 'moment'

I had to use default syntax:

import moment from 'moment'

This requires

    "allowSyntheticDefaultImports": true,

in tsconfig.json

Problem is i'm getting a new error now, in runtime... I'm pretty sure it's not related to your change but if you have clues it will be great

EXCEPTION: Error in ./IonicApp class IonicApp - inline template:0:34 caused by: No provider for ParamDecorator!ErrorHandler.handleError @ error_handler.js:45
error_handler.js:47ORIGINAL EXCEPTION: No provider for ParamDecorator!ErrorHandler.handleError @ error_handler.js:47
danbucholtz commented 7 years ago

@shlomiassaf,

Nice find with the allowSyntheticDefaultImports option. I'll play around with it - this sounds like the secret sauce that we need to make things work the way we want!

I'll have an update coming soon.

Thanks, Dan

danbucholtz commented 7 years ago

@shlomiassaf!

That option is working great so far!

Thanks, Dan

danbucholtz commented 7 years ago

I made my sample project public. Check it out.

https://github.com/driftyco/ionic-third-party-lib-example

Thanks, Dan

shlomiassaf commented 7 years ago

@danbucholtz Thanks! great progress.

It looks like the bundling issue is solved.

The cache issue is still there.

The node UUID lib uses the 3rd partycrypto library that rollup treats as external dependency. On the first run everything is fine but any code update done in watch mode will result in an error:

[21:23:35]  Error: Could not load crypto (imported by /Users/shlomiassaf/Desktop/Cisco/IONIC/ionic-third-party-lib-example/node_modules/node-uuid/uuid.js): ENOENT: no such file or directory, open 'crypto'

If we disable caching of the rollup bundle by commenting out this line code update works without a problem.

So, the problem is that caching somehow does not respect previous configuration.

For me this happened when using firebase as external module via the nodeModules plugin in rollup:

    nodeResolve({
      module: true,
      jsnext: true,
      main: true,
      browser: true,
      extensions: ['.js'],
      skip: [ 'firebase' ]
    })
shlomiassaf commented 7 years ago

@danbucholtz, still can't use angularfire2

shlomiassaf commented 7 years ago

@danbucholtz keep an eye on angular/angularfire2#565

awe-sim commented 7 years ago

Hi @danbucholtz,

I am facing similar issues over using external libraries like lodash.. I tried cloning the project you linked earlier (https://github.com/driftyco/ionic-third-party-lib-example).. The application runs and the libraries seem to work fine.. But in my IDE (SublimeText), I'm getting the following issues:

  1. For each Component ts file: Experimental support for decorators is a feature that is subject to change in a future release. Set the 'experimentalDecorators' option to remove this warning.
  2. For each of the library imports: Module "....../ionic-third-party-lib-example/node_modules/moment/moment" has no default export. In addition, IDE autocomplete for these library symbols is not working..

It seems to me I'm doing something stupid and overlooking something simple.. Could you please help in this regard?

danbucholtz commented 7 years ago

I'm thinking the sublime plugin you're using likely does not support the latest major version of Typescript (2.0). Give vscode a shot, it is maintained by Microsoft so it works incredibly well with Typescript.

Thanks, Dan

shlomiassaf commented 7 years ago

@awe-sim As a rule of thumb I always check console errors when running the complication and only then take the IDE errors seriously. No error in console, no issue.

It's hard for IDE plugins to keep up when changes are so often :)

awe-sim commented 7 years ago

@danbucholtz .. Thanks for pointing the issue out.. Yes the sublime plugin supported typescript v <2.0.. Using VSCODE now and its working good.. Thanks a lot.. =)

danbucholtz commented 7 years ago

@shlomiassaf, any leads on what's going on with AngularFire? Sounds like they may use a dependency that rollup can't handle? We may need to make WebPack an option if Rollup can't handle all libs. That would be unfortunate as performance would suffer.

Thanks, Dan

joshgarwood commented 7 years ago

I'm also having a similar issue with moment-timezone. I would love it if you guys reverted to webpack until rollup is more seriously vetted. The collective amount of time the community has wasted battling with this new build system is a rough indictment of the quality of this release. That, coupled with the general confusion on importing styles, iOS build failures with ionic package, et al... is just not consistent with the quality you guys typically ship with :/

Sorry to rant here, as I know it's not the place; it's just be a super frustrating 48 hours trying to migrate my beta11 project to rc0.

shlomiassaf commented 7 years ago

@danbucholtz I think you should have a talk with the firebase guys.

I'm not sure the issue is with the rollup build process, and if it is it's unique to the firebase build...

Just to recap I will sum up... There are 2 issues, one related to firebase the other is not.

Rollup can't handle firebase.

It should be noted that this happens when firebase is imported from angularfire2 I have not checked what happens if we omit angularfire2 and try to import firebase alone. @danbucholtz This should hint, if you can go with firebase without angularfire2 you should go to the angularfire2 guys and vice versa :)

To workaround for now see this issue comment

Rollup watch not working when using skip option in rollup-plugin-node-resolve

Basically I used skip to workaround the firebase issue. The problem is the cache bundle implemented in the bundle process, when cache is disabled there is no issue. When cache is enabled 1st compilation works fine but when updating the code rollup throws that he can't find firebase

Hope it helps, Thanks.

shlomiassaf commented 7 years ago

@mlynch & @danbucholtz About your note, moving to Webpack instead of Rollup. I think it is the right place to go to and should have been used from the start.

There are lot's of other reasons, really so many.

the app-scripts package is trying to implement things webpack does and does great! I don't see any benefit of using a custom rollup procces when it's clear that the direction of the community (at least angular) is webpack.

Why spending the time exploring lazy loading with your custom process or caching or any other issue when Webpack has it already baked in and if not the angular cli team has probably tackled it and solved it...

The angular team is going to release a lot of tools used by the Angular CLI as decoupled webpack plugins (see npm namespace @ngtools), this is what they said in angular connect.

I really thing you should reconsider and move to webpack.

BTW: My angular 2 library is using webpack for development and Rollup for UMD bundling so I can deploy an AOT ready library and an ES5 (UMD) library... It's possible to integrate the 2

danbucholtz commented 7 years ago

@shlomiassaf,

Don't use skip, used named exports to get Firebase working. What we need here is documentation. We've been able to get every library working with Rollup that we've tried, sometimes it has just taken a little configuration using named exports since the plugins have some weird dependency or weird way of exporting data that Rollup can't quite figure out.

Webpack is a nice tool but start-up time will be 2-3x slower from our tests. Do we sacrifice speed for everyone for the subset of people that use commonjs libs? I'm honestly not sure. This question has weighed heavily for me for a few days now.

Thanks, Dan

javebratt commented 7 years ago

Hey @danbucholtz I got a big production app updated to RC0 with Firebase 3 and AF2, took me a while but I got it working following this tutorial https://playcode.org/getting-started-with-ionic-2-rc0-firebase-3-angularfire-2/

Also, whenever I want to use the regular JS SDK instead of doing:

import * as firebase from 'firebase'

I just do declare var firebase

Before the @Component declaration and it works so I think you guys could look into a solution incorporating this instead of going back to webpack.

danbucholtz commented 7 years ago

Thanks for the tip @javebratt!

pskhodad commented 7 years ago

@danbucholtz, documentation/blog post will really help. Since you folks have been able to get most of the libraries working, I think it can save community some time figuring out things.

I faced issues with katex, providing named exports solved the issue. for me, one of the recent rollup optimizations seems to be causing problems, https://github.com/rollup/rollup-plugin-commonjs/issues/115. Was also facing issues with mathjs apparently related to node's built-ins.

danbucholtz commented 7 years ago

@pskhodad,

That may explain why things were "working better" for me a few weeks ago in earlier testing of Rollup. I was running my own custom branch of 4.x with the unambiguous grammar fix in. Thanks for pointing this out to us!

Thanks, Dan

ganySA commented 7 years ago

Hi

My two cents on this.

I have been updating my own app as well using various libraries some not the most common e.g:

and a couple of others.

I must agree with @shlomiassaf there are a large number of issues with the move to rollup. I have this feeling right now most people are "hacking" libs or the config to get things working - almost a trial and error.

My rollup config file looks like a mess with all the 'named' libs i have had to add in and also the amount of libs i have had to manually add into index.html + assets folder.

I understand there are benefits of rollup but i feel we have jumped the gun a little on it and maybe those benefits are not going to be realised by the large majority of users at this point because of the challenges to hack things together.

Hope your blog post comes soon because after two days of hacking away at various imports and rollup config settings i am going at the point of defeat!

pskhodad commented 7 years ago

There are couple of suggestions by @nolanlawson described in https://github.com/driftyco/ionic/issues/8356.

I guess this can provide best of both worlds,

"using Rollup in a very limited way (to produce a small CJS bundle), and then following up with Webpack/Browserify."

"Another possibility would be for the ionic-app-scripts to include a small whitelist of modules that are known to play nice with Rollup, and then use Browserify/Webpack for the rest."

masimplo commented 7 years ago

@danbucholtz when you say 2-3x slower startup, any idea why this is? Maybe with some configuration web pack can achieve the same results and being more "compatible", maybe it makes sense to invest in that instead of investing in rollup to play nice with everything. We have a lot to gain from webpack's popularity especially since it is being widely used in angular2.

danbucholtz commented 7 years ago

@masimakopoulos, Webpack is slower because it doesn't hoist all dependencies into the same scope. Long story short, each dependency is in it's own closure with WebPack/Browserify.

Nolan Lawson, a good friend of Ionic and PouchDB dev, wrote a great article on the subject. It's essential reading in my opinion. https://nolanlawson.com/2016/08/15/the-cost-of-small-modules/

The article shows not only the reduced bundle size of using something like Rollup, but also the performance improvement and startup time improvement.

We are not ruling out Webpack completely, but we're going to stick with Rollup for now. That's not to say the community couldn't create a Webpack config that others use, it's just not something we're going to pursue at this time. FWIW, we consulted with the Angular team before settling on Rollup.

Thanks, Dan

luchillo17 commented 7 years ago

@danbucholtz Will this repo ever support Webpack? i've been using it since alpha.43 and my project is basically binded to webpack at this point, changing to Rollup would mean scrapping up all my bundling pipeline so far and start learning Rollup, which basically means lots of time and effort will have to be invested in that process.

I honestly think it should be up to the developer which bundle tool to use, may ionic promote Rollup as the recommended way, but allow the developer the option to chose Webpack.

Is there a way for me to stick with Webpack right now with RC.0?

Also if i recall the beta.11 announcement in the Build Process Improvements section it says ionic would migrate to Webpack 2, isn't this the ionic blog?

shlomiassaf commented 7 years ago

@Luchillo From a developer point experience should be constant, so if Rollup is the choice it should be fully functional and high quality so I think getting there is more important then supporting 2 build systems.

Once the Rollup CLI is stable I think a webpack solution might come up... I agree with @danbucholtz on that.

I also believe the Webpack team will go towards performance improvements, at least getting close to rollup as much as possible but this will take some time.

@danbucholtz I solved all issues raised here with the new RC and the suggested solutions (named exports). I must admit, its not trivial and requires custom configurations which is a blocker for a lot of developers and will make them spend a lot of time on strange 3rd part integrations issues.

I think we can close this, maybe you can open a webpack issue for people to discuss...

Please close if you agree :)

henry74 commented 7 years ago

Not sure it makes sense to close this as I don't think using named exports in a config file that's loaded in the node_modules directory is a valid fix for this. I would hope there is a better outcome that doesn't involve modding config files on a case by case basis every time we add a new library to our project.

mlynch commented 7 years ago

@henry74 I agree with you. If we have to have a custom config for every single library, then rollup is not the right approach. However, what we're seeing is that many libraries have moved to the new, standard module format so they Just Work without you having to do anything. Additionally, many commonjs modules do work without extra configuration with default rollup plugins we're adding to app scripts by default. However, it's a few, admittedly important libraries, that get a bit too clever about their modules that make it difficult for rollup to understand them. For those, we have ways to make them work today until they move to the new standard.

The way we look at it is, if es2015 is the standard approach, shouldn't we pick it going forward? We'll assume that the most popular, highly used libraries, will soon adopt it if they haven't already. For those that don't, we have ways to make it work, and we're continuing to add more plugins and configs out of the box for rollup so you don't have to have a custom config.

I know it sucks in many ways, but it's not about rollup vs webpack, it's about a standard module format that the industry is adopting as we speak, vs custom ones that will be irrelevant much the way coffeescript is now that we have ES6 :D

We gain a lot of awesome benefits by having a standard module format that rollup can efficiently bundle, especially when we consider progressive web apps where loading a 2MB bundle file is just not acceptable (this is what we were seeing with webpack, along with noticeably slower boot times and time-to-first-render). Ionic developers keep asking us for faster apps and better performance. Well, this part of the runtime is really important for performance and perception of performance.

Anyways, that's the counter argument here that many Ionic team members believe in. It's not a universal opinion on the team, though, and even I have misgivings about it. However, in my testing, I've found that many if not most/all modules I've tested work fine, so I don't know.

That's my mini rant for the night :D

danbucholtz commented 7 years ago

@Luchillo,

You're free to use Webpack but we won't actively support you, have docs, etc. I am confident the community will create some solid options here. At this time, we are sticking with Rollup.

Yes, we did say we were going to go with Webpack 2. We weighed the pros and cons and settled on Rollup.

Thanks, Dan

mlynch commented 7 years ago

To add to what @danbucholtz said, the way we built all of this is that you can use other bundlers if you want. It won't be the official way, but you're entirely free to have any kind of script or build chain you want. If your code compiles and runs, that's all that matters if you're okay with the tradeoffs for your tool of choice!

luchillo17 commented 7 years ago

@danbucholtz Ok Rollup it is, so if i would like to stick for the time being with Webpack i wouldn't be able to make use of ionic-app-scripts? would i have to provide both the live reloading for ionic serve as well as the compiling that ionic run <platform> does after the JS build?

mixersoft commented 7 years ago

@shlomiassaf

BTW: My angular 2 library is using webpack for development and Rollup for UMD bundling so I can deploy an AOT ready library and an ES5 (UMD) library... It's possible to integrate the 2

I'm just doing dev, learning ionic2 + angular2 right now, and fast debugging with ts source maps is far more important to me than than a compact deployment. I jumped directly from gulp so webpack is still new to me. Can you (or someone else) share a webpack config that works with the RC0 build process for dev?

masimplo commented 7 years ago

@mlynch I see your point and sounds the way forward. Can we have some documentation for libraries that do not work out of the box with rollup today, on how we can make them work, so that we don't have to go through ten's of closed issues to find the solution each time? For instance I am trying to use pouchdb with my project but I am getting an error, I know it has been solved in some issue, but I think, given the popularity of the library, it would make sense to be more readily available.

luchillo17 commented 7 years ago

@mixersoft I'm with you about wanting a webpack example working with RC.0.

However in my case this is a little more complex, i already have an app in the Play Store and IOS store, so i really would like to stick with what i already know (Webpack) instead of figuring out Rollup quirks.

danbucholtz commented 7 years ago

@masimakopoulos,

I am writing that documentation as we speak. Expect for it to be posted today.

Thanks, Dan

masimplo commented 7 years ago

@danbucholtz awesome. Please add a link here when you are done. Also @shlomiassaf could you fix the typo in the title so it can be found more easily in the future?

mlynch commented 7 years ago

I gotchu

danbucholtz commented 7 years ago

@masimakopoulos,

It is looking like it may be one more day on the docs. I am doing a deep dive into the various issues and how to configure Rollup to circumvent them.

Thanks, Dan

danbucholtz commented 7 years ago

All,

What other libs are people having troubles with? I am trying to understand the breadth of the issues so I can document them well and come up with solutions.

Here's how you import Moment.

import moment from 'moment'

@shlomiassaf, Here's an Angularfire2 sample. Angularfire2 was not really any different of a solution than the pouchdb or some of the other commonjs libs where a deep dependency (not the lib itself) is getting clever with module.exports. Using namedExport options in the rollup config is the solution for now. We will very likely come up with a better solution in time. See the example here.

It is really easy to override the rollup config. We just did not do nearly a good enough job of documenting it. That is 110% on us. We really dropped the ball there and are working to resolve the issue by generating quality docs as we speak.

@mixersoft,

We are tracking the source map issue here.

Thanks, Dan

joshgarwood commented 7 years ago

The one that I need in particular is moment-timezone... It looks like this has been resolved with the latest ionic-app-scripts changes, but it may still be worth mentioning in the documentation you're working on.

Thanks for doing this by the way! We definitely need it :)

danbucholtz commented 7 years ago

@mlynch was able to get moment-timezone to work if I remember correctly.

I will add it to my list of libs to create a sample for.

Thanks, Dan