General Discussion #273

Open
danbucholtz opened this Issue Oct 28, 2016 · 42 comments

Projects

None yet
@danbucholtz
Contributor
danbucholtz commented Oct 28, 2016 edited

What is the build process missing? What could it do better? What use cases are we missing?

Basically, let us know how things are going and what we need to improve upon.

This thread is for general feedback and ideas. We can't fix what we don't know about! Please remember to be respectful and follow the Code of Conduct. Please use reactions instead of +1 comments.

Thanks,
Dan

@alan-agius4
Contributor
alan-agius4 commented Oct 28, 2016 edited

I'd very much like is you had SASS and HTML lints as well.

Also, maybe the option to disable lints as well. (I would be interested into switching off lints on watches to decrease the build time).

Another good thing would be an method to read args by key.

Example, I want to add a custom argv for my build. Such as --theme dark And it would be nice to have an available method to get the argv value. Something similar to yargs npm lib. Although I didnt manage to get it working with the app-scripts.

@Lingomat

Frankly, I'd prefer to double down on reliability. Be nice if watcher quit on ionic serve quit. If strangely old templates didn't start getting served, or otherwise watcher failing to pick up on changes.

Not sure what can be done to help with the often useless console dumps of errors that seem to all reference polyfill rather than any of your code. It just seems such a long time since you could just click on an error and see the line of code that went wrong...

Don't get me wrong, I'm loving the improvements, I'm just saying, fancy features feel like they ought to come later.

@danbucholtz
Contributor

@Lingomat,

110% agreed. @ionic/app-scripts@0.0.38 published today provides a bunch of stability fixes and speed improvements. Stability and speed improvements are our focus right now.

Thanks,
Dan

@Lingomat

Thanks Dan, really appreciate your work.

@marcoturi
Contributor
@Chris1308

Hey,

another improvement could be the handling of tsconfig file. There is an exlude (node_modules) and it works fine with ionic serve. But when i start ionic run android, i got errors, because the file is copied to tmp folder and the path to node_modules is then '../node_modules'. The angular-cli creates the tsconfig-file inside the src folder and the path is always '../node_modules'. Maybe something ionic can adapt.

@Lingomat

Seriously though, the console error messages at present are frequently just useless. Sometimes you can spot the name of a class in the dump of internals about polyfills.ts but sometimes there's not even that.

I can't be alone in this surely?

@danbucholtz
Contributor

@Lingomat,

Can you give me an example of an error? And this is at runtime while using the app, not build time, correct?

Thanks,
Dan

@Lingomat
Lingomat commented Oct 31, 2016 edited

Using the app yes. I'm in PWA land. This one just spat out of Firefox. At least I know the component. It hasn't happened for a couple of days, but I'll occasionally get a dump that doesn't tell me that, this is the worst case. Next time one happens I'll paste.

Unhandled Promise rejection: _b is undefined ; Zone: angular ; Task: Promise.then ; Value: TypeError: _b is undefined
Stack trace:
LangEditComponent</LangEditComponent.prototype.initialize/<@http://localhost:8100/build/main.js:93567:54
O</d</t.prototype.invoke@http://localhost:8100/build/polyfills.js:3:13398
NgZoneImpl/this.inner<.onInvoke@http://localhost:8100/build/main.js:104271:28
O</d</t.prototype.invoke@http://localhost:8100/build/polyfills.js:3:13336
O</v</e.prototype.run@http://localhost:8100/build/polyfills.js:3:10768
h/<@http://localhost:8100/build/polyfills.js:3:8887
O</d</t.prototype.invokeTask@http://localhost:8100/build/polyfills.js:3:14018
NgZoneImpl/this.inner<.onInvokeTask@http://localhost:8100/build/main.js:104262:28
O</d</t.prototype.invokeTask@http://localhost:8100/build/polyfills.js:3:13946
O</v</e.prototype.runTask@http://localhost:8100/build/polyfills.js:3:11370
i@http://localhost:8100/build/polyfills.js:3:7999
t/this.invoke@http://localhost:8100/build/polyfills.js:3:15182
 LangEditComponent</LangEditComponent.prototype.initialize/<@http://localhost:8100/build/main.js:93567:54
O</d</t.prototype.invoke@http://localhost:8100/build/polyfills.js:3:13398
NgZoneImpl/this.inner<.onInvoke@http://localhost:8100/build/main.js:104271:28
O</d</t.prototype.invoke@http://localhost:8100/build/polyfills.js:3:13336
O</v</e.prototype.run@http://localhost:8100/build/polyfills.js:3:10768
h/<@http://localhost:8100/build/polyfills.js:3:8887
O</d</t.prototype.invokeTask@http://localhost:8100/build/polyfills.js:3:14018
NgZoneImpl/this.inner<.onInvokeTask@http://localhost:8100/build/main.js:104262:28
O</d</t.prototype.invokeTask@http://localhost:8100/build/polyfills.js:3:13946
O</v</e.prototype.runTask@http://localhost:8100/build/polyfills.js:3:11370
i@http://localhost:8100/build/polyfills.js:3:7999
t/this.invoke@http://localhost:8100/build/polyfills.js:3:15182
Error: Uncaught (in promise): TypeError: _b is undefined
Stack trace:
s@http://localhost:8100/build/polyfills.js:3:8546
h/<@http://localhost:8100/build/polyfills.js:3:8918
O</d</t.prototype.invokeTask@http://localhost:8100/build/polyfills.js:3:14018
NgZoneImpl/this.inner<.onInvokeTask@http://localhost:8100/build/main.js:104262:28
O</d</t.prototype.invokeTask@http://localhost:8100/build/polyfills.js:3:13946
O</v</e.prototype.runTask@http://localhost:8100/build/polyfills.js:3:11370
i@http://localhost:8100/build/polyfills.js:3:7999
t/this.invoke@http://localhost:8100/build/polyfills.js:3:15182
@danbucholtz
Contributor

@Lingomat,

Can you open a separate issue with this and any relevant information or a repository that we can use to recreate the issue? I am unclear if this is an issue in your code, or just too short of a stack trace.

Thanks,
Dan

@alteredorange
alteredorange commented Nov 1, 2016 edited

Here's a more "newbie" perspective. I really like Ionic because it seems to be the easiest way to create mobile apps. I'm played around with Angular 1 and 2, am pretty familiar with javascript, but I'm no expert. I like Ionic because most things just work (in theory). Right now the problem is A) I really don't want to spend time learning Ionic 1 and Angular 1, because those are getting replaced but B) Ionic 2 isn't ready yet/and I have no idea when the stable release will be ready.

As mentioned, Ionic has a lot of things that work out of the box like auth, push notifications and hopefully sometime soon I'll get an invite to that new json backend. Yet there's not a starter template with all of this baked in!

It would be really nice to have the standard blank template, the tabs template, and a more "complete" template with boilerplate auth (just plop in your own keys) and a really basic push notification set up. That way it would all be working and easy to see, rather than slogging through a bunch of docs which may or may not work by the time I finish everything.

Anyways, those have been my biggest hurdles so far. I just wish everything was ready righ now :)

Oh, and as far as webpack goes, it's a lot smoother now working with firebase, rollup needed a lot of annoying config.

Keep up the good work, thanks!

@fstof
fstof commented Nov 1, 2016

Good job on switching to Webpack.

What I would love to see is the build not just including all component .scss files into a single global css.
Rather have them work the way Angular2 intended (ie view encapsulation via shadow dom and all those goodies)

If there is a way in getting that, please point me in the right direction

Thanks

@Barryrowe

Is there an existing guide/example to properly handle unit tests with the new configurations?

Assuming a project has *-spec.ts files as siblings to the files they test. I know we had to customize our webpack setup with the beta releases, and this is something that is going to be very important as we move from beta.11 to rc.0+.

@danbucholtz
Contributor

@Barryrowe,

We're planning on adding unit test support in app-scripts in the coming weeks/months. I haven't really thought through the best practices yet to be honest.

Thanks,
Dan

@jgw96
Member
jgw96 commented Nov 1, 2016 edited

@fstof Good question about shadow dom. So there are a few reasons we dont use shadow dom with our components. First, it is my understanding that Angular 2 uses the older v0 shadow dom spec that is only supported in chrome. Since Angular 2 has come out there has been the new v1 shadow dom spec that has come out that all browsers have agreed to implement and is already in Chrome and Safari 10. Because of this I don't think the use of Shadow dom in Angular 2 is as useful. The second reason we don't use shadow dom is because we already have our own implementation of css scoping by using sass. Now, something i can agree on looking at is how we put all our css in one file. For Ionic 2 apps running in cordova it makes sense, but for an Ionic 2 app deployed as a PWA on a server that is running http 2 (starting to be the majority of servers nowadays) you can actually get faster load time by loading smaller individual files than one big bundle. Hope this explains everything well!

@Barryrowe
Barryrowe commented Nov 1, 2016 edited

@danbucholtz Thanks for the update. This is good to hear.

One of the big concerns I have is if the app-scripts lib is looking to pack everything via the webpack API without a webpack.config file in each project. There are a lot of IDEs, or other test runners that would need to pull in the webpack config for tests in order to function properly. (Karma and WallabyJS are two that we use in particular).

Whatever the solution, it probably also needs to be flexible on defining the naming convention for test files (*-spec.ts, *.spec.ts, *Test.ts, etc.), and where they live. I'm not sure there's a consensus on spec files being siblings to the files they test, or if they should live in a "test" folder separated from the app source.

@fstof
fstof commented Nov 2, 2016

Thanks @jgw96
Agreed. Angular2's native shadow dom is pretty much useless in the real world, but it does a pretty good job with its "emulated" implementation of it.

My concern is really that we are effectively losing that feature with the current build process.
Yes, I can get it by specifying styles: ['.bob {color: red}']in my @Component decorator, but then you don't have the advantage of sass (or at least separation).

I think being able to specify styleUrls:['./component.scss'] would be extremely beneficial.
I haven't tried the normal webpack way with the sass-loader and raw-loader combination yet (I doubt it would work, as the scss files are not copied to {{TMP}} and thus not available during bundling)

Maybe there is a way of having the scss file loaded individually for each component (via the sass/raw-loader combination) if specified in styleUrls, or included in the global scss if its not specified

@KyleMcNutt

@Barryrowe @danbucholtz This is the exact headache that I'm having in a project right now. I used Rollup in my Karma configuration via the karma-rollup-plugin because app-scripts was using Rollup, but now that app-scripts has changed to using Webpack (which I really like) I'm having to deal with my Karma configuration all over again, and it's not fun or easy at all.

This is really compounded by the lack of documentation on Unit Testing within the Ionic framework and is something that I would love to see greatly improved. Ideally this would also come with a set of built in useful mocks for Ionic components like NavController, AlertController, etc.

@marcoturi
Contributor
marcoturi commented Nov 2, 2016 edited

@Barryrowe and @KyleMcNutt meanwhile if you need an example for unit testing, look here.

@OnnoGeorg

Back to shorter build times, finally :-) I am running @ionic/app-scripts@0.0.39 and the build process with ionic serve got faster by a factor of 2 compared to the former rollup based versions, thanks! This helps a lot.
And thumbs up for the approach of keeping the build process a simple as possible.

@ryanlogsdon

Hi, Since Rollup is still default, would you consider adding a CLI switch for WebPack2, so we don't have to edit the tsconfig.json file every time? "--wp2"

@danbucholtz
Contributor

@ryanlogsdon, Rollup is not the default. If you have an existing config file or pass the --rollup option to the build, you'll use Rollup. Otherwise, it's Webpack.

Thanks,
Dan

@maxtuzz
maxtuzz commented Nov 3, 2016

Hi all,

Trying to upgrade to latest ionic-app-scripts. My project is serving fine - no problems at all, but as soon as my browser window opens I get a console error:

Uncaught Error: Cannot find module "."
    at webpackMissingModule (http://localhost:8100/build/main.js:99667:69)
    at http://localhost:8100/build/main.js:99667:147
    at Object.<anonymous> (http://localhost:8100/build/main.js:99672:3)
    at __webpack_require__ (http://localhost:8100/build/main.js:20:30)
    at Object.<anonymous> (http://localhost:8100/build/main.js:79074:86)
    at __webpack_require__ (http://localhost:8100/build/main.js:20:30)
    at Object.<anonymous> (http://localhost:8100/build/main.js:78475:85)
    at __webpack_require__ (http://localhost:8100/build/main.js:20:30)
    at Object.<anonymous> (http://localhost:8100/build/main.js:116083:70)
    at __webpack_require__ (http://localhost:8100/build/main.js:20:30)

Am I missing something? I've tried removing and rebuilding node_modules but this problem persists for me.

Thanks,
Max

@alan-agius4
Contributor
alan-agius4 commented Nov 3, 2016 edited

Thats seems to be an AOT issue, check your dependencies. I had a similar
issue and the reason was that there was no 'module'property in the
package.json of one of my dependencies and was reading the UMD rather than the ES2015 (which was APT ready)

You might also be using a library which is not AOT ready

On Thursday, 3 November 2016, Max Tuzzolino notifications@github.com
wrote:

Hi all,

Trying to upgrade to latest ionic-app-scripts. My project is serving fine

  • no problems at all, but as soon as my browser window opens I get a
    console error:

Uncaught Error: Cannot find module "."
at webpackMissingModule (http://localhost:8100/build/main.js:99667:69)
at http://localhost:8100/build/main.js:99667:147
at Object. (http://localhost:8100/build/main.js:99672:3)
at webpack_require (http://localhost:8100/build/main.js:20:30)
at Object. (http://localhost:8100/build/main.js:79074:86)
at webpack_require (http://localhost:8100/build/main.js:20:30)
at Object. (http://localhost:8100/build/main.js:78475:85)
at webpack_require (http://localhost:8100/build/main.js:20:30)
at Object. (http://localhost:8100/build/main.js:116083:70)
at webpack_require (http://localhost:8100/build/main.js:20:30)

Am I missing something? I've tried removing and rebuilding node_modules
but this problem persists for me.

Thanks,
Max


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#273 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AQv-Wv2H1pkFNrQi8psRGMadCA-1U-6Hks5q6mTvgaJpZM4KjrnZ
.

@maxtuzz
maxtuzz commented Nov 3, 2016 edited

Thanks for your reply. I've played around with my dependencies a bit and can't seem to get it right. I'm not too sure there is much out of the ordinary with mine:

{
  "name": "my-app",
  "description": "An Ionic project",
  "scripts": {
    "build": "ionic-app-scripts build",
    "watch": "ionic-app-scripts watch",
    "serve:before": "watch",
    "emulate:before": "build",
    "deploy:before": "build",
    "build:before": "build",
    "run:before": "build"
  },
  "dependencies": {
    "@angular/common": "2.1.1",
    "@angular/compiler": "2.1.1",
    "@angular/compiler-cli": "2.1.1",
    "@angular/core": "2.1.1",
    "@angular/forms": "2.1.1",
    "@angular/http": "2.1.1",
    "@angular/platform-browser": "2.1.1",
    "@angular/platform-browser-dynamic": "2.1.1",
    "@angular/platform-server": "2.1.1",
    "@ionic/storage": "1.1.6",
    "ionic-angular": "2.0.0-rc.2",
    "ionic-native": "2.2.3",
    "ionicons": "3.0.0",
    "rxjs": "5.0.0-beta.12",
    "zone.js": "0.6.21"
  },
  "devDependencies": {
    "@ionic/app-scripts": "0.0.39",
    "typescript": "^2.0.3"
  },
  "cordovaPlugins": [
    "cordova-plugin-statusbar",
    "cordova-plugin-device",
    "ionic-plugin-keyboard",
    "cordova-plugin-whitelist",
    "cordova-plugin-geolocation"
  ],
  "cordovaPlatforms": [
    "ios",
    "android"
  ]
}

Pretty standard. No third party dependencies outside of the normal set Ionic comes with. I use the Ionic conference app package.json as a reference. The only inconsistencies I can see are around Cordova configuration, and my not using moment.js.

I've also confirmed that the error also occurs with angular 2.0 alongside 2.1.1.

Max

@mtycholaz

@Barryrowe and @KyleMcNutt I manually edited the build script folder to exclude testing files. It wasn't hard. Practically a one-liner.

@maxtuzz
maxtuzz commented Nov 5, 2016

After playing around a while the root cause of my error was a dodgy import Webstorm auto-filled out for me. Sometimes it worries me how not very specific JavaScript errors can be sometimes.

Thanks guys, definitely a big improvement!

@architruc

Hi all,

I believe a great next step would be to "tree-shake" the resources like fonts/icons, CSS... I don't know how realistic it is (and it is not only for Ionic apps), but since it is a general topic for ideas of improvements, I share my crazy ideas :)

I love the concept of tree shaking and there have been great improvements around JS tree-shaking recently. Since Ionic builds with AOT, now I am checking the application size, I feel like most of the app size is the bunch of icons, styles and other resources bundled with Ionic (at least for a small app). We rarely use most of the icons on the same time, same for the style. For sure it would reduce greatly the bundle size to keep only the part the app uses.

But it would also be very challenging as it would involve being aware of the generated HTML to detect the styles and resources referenced. Not even mentioning dynamic changes through JavaScript... Maybe a kind of Angular 2 universal or template compiler, relying on ng wrappers of native DOM elements?

I'm still dreaming of a day when the app bundle would contain really only what is used at run-time :)

@janthonyeconomist

A very basic addition I would like is an environment (release vs debug) specific application configuration system, so that my development builds can hit development backends while my production builds will hit production backends.

@danbucholtz
Contributor

@janthonyeconomist,

This feature will land in master today, and be out for general usage hopefully by tomorrow.

Thanks,
Dan

@Barryrowe

@mtycholaz I appreciate that you're reporting you got this working, but can you point me to the right files? I'm looking through this and the default webpack file being used doesn't appear to have a typescript loader defined, so I'm not sure where to modify and add my exclude: property.

@danbucholtz
Contributor

@janthonyeconomist,

We decided we want to revisit the hooks API that I mentioned yesterday. It won't hit today after all.

Thanks,
Dan

@mtycholaz
mtycholaz commented Nov 22, 2016 edited

@Barryrowe

UPDATE: I'm working on RC1 right now, so this may be a little different from RC2 or RC3

Open up node_modules\@ionic\app-scripts\dist\ngc.js and change line 144 from
shouldInclude = (helpers_1.endsWith(filePath, '.ts') || helpers_1.endsWith(filePath, '.html'));
to this
shouldInclude = !helpers_1.endsWith(filePath, '.test.ts') && (helpers_1.endsWith(filePath, '.ts') || helpers_1.endsWith(filePath, '.html'));

@Mokto
Contributor
Mokto commented Nov 28, 2016

It has already been mentioned by @marcoturi, but have you planned to support HMR ?

@Barryrowe

Is there a straightforward way to run an ionic build WITHOUT using the AoT compiler options? The AoT compiler is just not ready for widespread usage yet, and so many third-party libraries aren't published with the required metadata.json files, or are otherwise incompatible with AoT compiling.

I'm hoping I've just overlooked a simple flag.

@Mokto
Contributor
Mokto commented Dec 1, 2016
@danbucholtz
Contributor

@Barryrowe,

Yes. Use the --dev option at the end of your build script.

We are going to change this around in the coming weeks too. dev by default, AoT is opt in. For now, do the following:

For example (off the top of my head)

"ionic:build" : "ionic-app-scripts build --dev"

Thanks,
Dan

@Barryrowe

@danbucholtz perfect sounds like a good decision to me. AoT seems pretty temperamental still. Thanka

@Barryrowe

@danbucholtz Until AoT is made opt in, using the --dev flag is going to have more side effects than just disabling AoT correct? This resets the IONIC_ENV to "dev" instead of "prod". Do you know what other implications this has outside of using the main.dev.ts file instead of the main.prod.ts and turning off AoT?

@danbucholtz
Contributor

It will disable minification and AoT for now.

Thanks,
Dan

@Barryrowe

@danbucholtz back at the beginning of November you mentioned you all are planning to add unit test support. Any updates to speak on this?

I'm trying to hack through it right now with v0.0.45 of ionic-app-scripts. I'm just going to try to describe some of the issues I'm having in case they are useful for your design approach:

  • as it stands right now, I would have to exclude my spec files via tsconfig.json when running the app, otherwise I end up with compile errors because jasmine isn't provided, nor are the types.

    • I can't leave this by default, because this would mean I have to keep updating tsconfig back to include them when I want to work in my specs so my editor, in my case VS Code, is parsing them with tsc
    • To get around this I've added the following code to node_modules/@ionic/app-scripts/dist/transpile.js line 247 (just before it parses the config)
    if(tsConfigFile.config){
            var excludePatterns = ["**/*-spec.ts", "mocks"];
            if(tsConfigFile.config.exclude){
                tsConfigFile.config.exclude.push(...excludePatterns);                    
            }else{
                tsConfigFile.config.exclude = excludePatterns;
            }
        }
    //This would really want to pull from an "transpile_excludes" or similar property in package.json, or another config file
  • For runners that need their own build process, I'm having difficulty determining the best way to reproduce the transpile step in a reproducible way. Before webpack was doing the transpile via a typescrpt loader, and were clearly providing the polyfills via a spec-bundle or similar setup. Now all of this is hidden in the app-scripts files, so getting a test webpack setup for runners is a chore.

    • Whatever solution is determined, the tests need to be able to be run with other runners without a ton of work
@danbucholtz
Contributor

Yep, we intend to add unit testing support. It is something we will add in the coming months.

Thanks,
Dan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment