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

Update project to Ionic2-rc0 #148

Closed
masimplo opened this issue Sep 29, 2016 · 39 comments
Closed

Update project to Ionic2-rc0 #148

masimplo opened this issue Sep 29, 2016 · 39 comments

Comments

@masimplo
Copy link

I know it's just been a few hours that rc0 was released, but I am betting everyone is moving their projects over - I know I am. Any plan of action for tests to work with the new system?

@lathonez
Copy link
Owner

Yeah, it's a massive amount of work even for a tiny app like this.

https://github.com/driftyco/ionic/blob/master/CHANGELOG.md#steps-to-upgrade-to-rc0

I will be doing it as I have a huge closed source project to migrate based on the experiences here. Can't say I'm looking forward to it.

With regard to testing, I'm not sure if you've seen this comment, implying that tests should work "out of the box" using ng's CLI. Unsure whether this would be working on rc0 (no indication from the change log that it would), but it's definitely the way to go.

I will start work on this on Wednesday (5th October) at the absolute earliest, perhaps later next week.

If you have any pointers to share when migrating your own projects (maybe some gotchyas that aren't clear from the CL), or have found any useful resources for the migration, I will be eternally grateful.

@masimplo
Copy link
Author

Wasn't aware of that comment you mention, but I guess it was a matter of time since ionic2 provided an out of the box solution for testing our code. To tell you the truth I am both excited and depressed that rc0 came out. Excited that breaking changes are over and most cool features are in, depressed as the amount of work to migrate out huge project is just obscene.
I will be starting right away, so if I find any gotchas not documented I will be sure to share them.

@lathonez
Copy link
Owner

Excited that breaking changes are over and most cool features are in, depressed as the amount of work to migrate out huge project is just obscene.

My thoughts exactly. We'll get there..

@mavogel
Copy link
Contributor

mavogel commented Oct 2, 2016

Just took a look how project generated with the latest angular-cli.beta16 set up the unit and e2e test environment.

I though about giving it a try to add it to the ionic-conference-app but then I found this issue on which @masimakopoulos replied as well.

What do you think: wait until they add it as a task in the ionic-app-scripts or write an own "test": "ng test" task in the package.json?

@murraybauer
Copy link

I'm hoping the Ionic CLI will better leverage the Angular CLI (for both testing and i18n internationalisation). The Angular CLI will always be ahead. Ionic of course needs to roll back too Webpack, which they seem to be considering (?)

I use clicker now and it was a great help, but does something like clicker still need to exist long term?

@ovitrif
Copy link

ovitrif commented Oct 3, 2016

Clicker will always help, considering the ngrx/store example within 👍

So, migration will start this week at best?!

Please keep us up-to-date with your progress.

@cescoferraro
Copy link

I was sad because going with rollup.js means moving further away from the angular-cli

@lathonez
Copy link
Owner

lathonez commented Oct 4, 2016

Thanks for all the comments.

but does something like clicker still need to exist long term?

Frankly I think it's ridiculous that this project still seems to be the benchmark for testing an Ionic 2 project, since I first wrote it way back when on beta.0.

Hopefully in the not-to-distant future we'll all be doing the same thing, using the angular-cli (albeit via some ionic test command)

Ionic of course needs to roll back too Webpack, which they seem to be considering (?)

I was expecting webpack in rc0 (see also #131), and too was disappointed to see rollup. No idea if Ionic will go back to webpack?

ionic-team/ionic-app-scripts#51 is an interesting read, thanks for the reference. I am really surprised that we've got to a release candidate on a framework that basically nullifies all the testing goodies we get from ng2.

Please keep us up-to-date with your progress.

Will do. I'd like to get as close to angular-cli as possible. Let's see what happens.

@lathonez
Copy link
Owner

lathonez commented Oct 4, 2016

Made a branch rc0 for this:

https://github.com/lathonez/clicker/compare/rc0?expand=1

@masimplo
Copy link
Author

masimplo commented Oct 4, 2016

I am really surprised that we've got to a release candidate on a framework that basically nullifies all the testing goodies we get from ng2.

My thoughts exactly!

@mfdeveloper
Copy link

Hello @lathonez !! I've created a structure to allow unit tests with RC.0. By now, only Tabs scaffold doesn't working. Please, see this discussion: marcoturi/ionic-boilerplate#1

Gist: https://gist.github.com/mfdeveloper/d9349dea78ba34a36adc7ada56ffd0c0

@lathonez
Copy link
Owner

lathonez commented Oct 4, 2016

Looks interesting thanks!

On 4 Oct 2016 23:38, "Felipe Michel" notifications@github.com wrote:

Hello @lathonez https://github.com/lathonez !! I've created a structure
to allow unit tests with RC.0. By now, only Tabs scaffold doesn't
working. Please, see this discussion: marcoturi/ionic-boilerplate#1
marcoturi/ionic-boilerplate#1

Gist: https://gist.github.com/mfdeveloper/d9349dea78ba34a36adc7ada56ffd0
c0


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

@marcoturi
Copy link

marcoturi commented Oct 5, 2016

last release of https://github.com/marcoturi/ionic2-boilerplate has e2e and unit tests working

@tja4472
Copy link
Contributor

tja4472 commented Oct 6, 2016

Hi

Here is my Ionic RC0 plus latest ngrx app.
https://github.com/tja4472/ngrx-ionic-angularfire

hth
Tim

@lathonez
Copy link
Owner

Can't figure out how to get ngrx working on 2.2.1, which seems to be required to import into the app module. Might have to go back to the drawing board with it.

@lathonez
Copy link
Owner

Thanks in a very large way to @tja4472 on #152 the app finally starts.

@lathonez
Copy link
Owner

The good news is that the app is working (not tests yet - but I'm getting there), on rc0.

The bad news is that I've had to remove ngrx.

@tja4472 - Tim apologies for rolling this back. The gap in my knowledge is too big to maintain this repo running ngrx.

Moreover, I have a massive closed source project that needs migrating based on experiences here and I've no time to learn the reactive stuff properly whilst doing it.

After the dust has settled, I'm going to re-implement ngrx myself from scratch so I can learn the principles properly. I'll be using ngrx-ionic-angularfire as a guide.

Steve

@lathonez
Copy link
Owner

Latest commit on rc0 has a proof of concept using ng-cli for tests only:

x220:~/code/clicker(rc0)$ npm test
(node:18859) fs: re-evaluating native module sources is not supported. If you are using the graceful-fs module, please update it to a more recent version.

> Clicker@2.0.0 test /home/lathonez/code/clicker
> ng test

Could not start watchman; falling back to NodeWatcher for file system events.
Visit http://ember-cli.com/user-guide/#watchman for more info.
12 10 2016 18:45:30.828:WARN [karma]: No captured browser, open http://localhost:9876/

WARNING in ./~/@angular/core/src/linker/system_js_ng_module_factory_loader.js
45:15 Critical dependency: the request of a dependency is an expression

WARNING in ./~/@angular/core/src/linker/system_js_ng_module_factory_loader.js
57:15 Critical dependency: the request of a dependency is an expression
12 10 2016 18:45:30.842:INFO [karma]: Karma v1.2.0 server started at http://localhost:9879/
12 10 2016 18:45:30.842:INFO [launcher]: Launching browser Chrome with unlimited concurrency
12 10 2016 18:45:30.959:INFO [launcher]: Starting browser Chrome
12 10 2016 18:45:32.103:INFO [Chrome 53.0.2785 (Linux 0.0.0)]: Connected on socket /#2bfrbI6eI76QgaNHAAAA with id 83995976

START:
Chrome 53.0.2785 (Linux 0.0.0): Executed 0 of 1 SUCCESS (0 secs / 0 secs)
  Pages: Page2
Chrome 53.0.2785 (Linux 0.0.0): Executed 1 of 1 SUCCESS (0.807 secs / 0.798 secs)

Finished in 0.807 secs / 0.798 secs

SUMMARY:
✔ 1 test completed

@larssn
Copy link

larssn commented Oct 12, 2016

@lathonez Just wanted to say thank you for the work you do on this. I really want to use tests in my RC0 project. Surprised Ionic 2 doesn't support it out of the box.

@lathonez
Copy link
Owner

All unit tests are now passing on the rc0 branch, built on top of ng-cli as opposed to anything to do with ionic / rollup.

I'm not sure how far along Ionic are in getting the tests sorted out or what direction they are going. Last I read was that they were trying to get as close to ng-cli as possible (that was before the moved to rollup over webpack).

At present I'd sooner align myself with ng-cli than move to another variant of Ionic's build process.

TODO:

  • tidy up / sort out CI
  • merge back
  • document
  • e2e!

@chron0
Copy link
Contributor

chron0 commented Oct 15, 2016

@lathonez: Thanks for all the efforts and keeping it alive. Looking forward to have testing in RC+ again. RC1 fortunately hasn't brought any more breaking changes so far :)

@lathonez
Copy link
Owner

updated to rc1

@lathonez
Copy link
Owner

lathonez commented Oct 18, 2016

ionic-native (for posterity)

Importing anything from ionic-native during testing gives hundreds of warnings:

WARNING in ./~/ionic-native/dist/esm/index.js
Cannot find source file '../../src/index.ts': Error: Can't resolve '../../src/index.ts' in '/home/lathonez/code/clicker/node_modules/ionic-native/dist/esm'
 @ ./src/app/app.component.ts 9:2587-2610
 @ ./src/app/app.spec.ts
 @ ./src \.spec\.ts
 @ ./src/test.ts

I believe this is because the source maps for ionic-native are broken: angular-redux/store#64

Custom webpack config to exclude ionic-native from source map loading would be required to remove the warnings, however this isn't going to be an option for some time: angular/angular-cli#1656

So, if you want to test code using ionic-native, you need to live with the warnings for the time being: angular/angular-cli#2364

@lathonez
Copy link
Owner

lathonez commented Oct 18, 2016

WARNING in ./~/@angular/core/src/linker/system_js_ng_module_factory_loader.js
45:15 Critical dependency: the request of a dependency is an expression

WARNING in ./~/@angular/core/src/linker/system_js_ng_module_factory_loader.js
57:15 Critical dependency: the request of a dependency is an expression

are fixed in: angular/angular-cli#2362

and fixed here: 118dd99

@lathonez
Copy link
Owner

Updated the unit testing blog: lathonez/lathonez.github.io@c38cb54

http://lathonez.com/2016/ionic-2-unit-testing/

@nicopeeters
Copy link

nicopeeters commented Oct 19, 2016

Hi, I have also tried to upgrade to RC and I'm getting this output:

19 10 2016 16:15:04.632:INFO [karma]: Karma v1.3.0 server started at http://localhost:9876/
19 10 2016 16:15:04.633:INFO [launcher]: Launching browser Chrome with unlimited concurrency
19 10 2016 16:15:04.967:INFO [launcher]: Starting browser Chrome
19 10 2016 16:15:08.464:INFO [Chrome 53.0.2785 (Windows 10 0.0.0)]: Connected on socket /#Atyk6forXIq7q454AAAA with id 90544399

START:
Chrome 53.0.2785 (Windows 10 0.0.0) ERROR

Chrome 53.0.2785 (Windows 10 0.0.0) ERROR


Finished in 3.406 secs / 0 secs

SUMMARY:
√ 0 tests completed

My home.spec.ts file:

import { ComponentFixture, TestBed, async } from '@angular/core/testing';
import { TestUtils }                        from '../../test';
import { HomePage }                         from './home';

let fixture: ComponentFixture<HomePage> = null;
let instance: any = null;

describe('Pages: HomePage', () => {

    beforeEach(() => {
        TestUtils.configureIonicTestingModule([HomePage]);
        fixture = TestBed.createComponent(HomePage);
        instance = fixture.debugElement.componentInstance;
    });

    it('should create page2', async(() => {
        expect(instance).toBeTruthy();
    }));
});

In Google Chrome, it says that my browser is idle:

Karma v1.3.0 - connected
Chrome 53.0.2785 (Windows 10 0.0.0) is idle

Do you have any idea what may cause this?

@flash716
Copy link

This is great, any guidance on including packages like moment or lodash? In the RC the imports changed from:

import * as moment from 'moment';
import * as _ from 'lodash';

to

import moment from 'moment';
import _ from 'lodash';

The new way works for building/running but fails unit tests.

TypeError: moment_1.default is not a function
TypeError: lodash_1.default is not a function

The old way fails building but passes unit tests.

bundle failed: Cannot call a namespace ('moment')

@cmhand
Copy link

cmhand commented Oct 19, 2016

@flash716 I've filed an issue on angular-cli for that:

angular/angular-cli#2787

@lathonez
Copy link
Owner

FYI - I've been told by @marcoturi (via Dan from Ionic) that Ionic are eventually going with rollup for testing.

I'm not following Ionic down the (4th?) build rabbit hole since we first started testing. Whilst using ng-cli is a pain (as we need to include another cli), I think the benefits of being in sync with the rest of angluar2 community clearly outweigh the drawbacks.

If Ionic eventually release some testing framework that seems to be stable then I may look to move back to it, however testing is an afterthought (or less, at the moment) for Ionic and I'd rather have the support of ng-cli.

@lathonez
Copy link
Owner

@cmhand thanks look forward to the outcome.

@nicopeeters can you share the project or a repro against clicker?

@marcoturi
Copy link

marcoturi commented Oct 20, 2016

@lathonez I'm adding some more info. In the last 2 days i was trying to switch my boilerplate from running tests through rollup to webpack. The reasons are those:

  1. Without the possibility to use require() syntax in karma-shim.js to load all spec (ie var testContext = require.context('../src', true, /\.spec\.ts/);) i had to use a little hack: running a watch task on spec files (before karma), that replace inside karma-shim the list of spec files. It works good, but i don't like it vary much. See here and here.
  2. This issue with istanbul.
  3. I would like to try to add HMR in the future if possible.

After importing the new app-script i tried to get karma working with webpack. And i found a problem with the current syntax for include templates in components. See this issue. For me the solution was to replace the syntax templateUrl: 'home.html' to template: require('home.html). I asked to Dan what was the best at this point. Stick up to rollup or switch to angular cli or webpack to run tests. The answer was "stick to rollup" as it is the future bundler, and also avoid the use of require in code as it will not be compatible in the future when we will switch only to rollup".

To me make sense to use webpack only if avoiding require syntax as it will not be supported in the future. I'm vary interested to listen any thought about this :)

Edit: i fixed the template bug, now all it's working fine with webpack and ionic 2.

@ey-aroberts
Copy link

Incase anyone is using chrome 55. Here is a fix for tests not working. angular/angular-cli#2125 (comment)

@lathonez
Copy link
Owner

@ztecharoberts nice thank you

@cmhand
Copy link

cmhand commented Oct 24, 2016

As an update, the issue I filed against angular-cli about not being able to run ng-test was closed. The problem is that the default moment.js file really doesn't have a default export and allowSyntheticDefaultImports only works for commonjs and systemjs modules.

A workaround is to import from 'moment/src/moment.js' as opposed to moment, because this file does actually have a default export specified. In addition, for this workaround to work, you may have to move a typings file in this directory.

Really, I think it just means that the ionic-app-scripts team needs to support the import * as moment syntax.

@lathonez
Copy link
Owner

As e2e is back in I think I can finally close this.

Thanks all for your patience and assistance.

fwiw I'm really happy with where we are now. Running an identical setup to ng-cli has been super helpful already in a number of issues and other problems I've had getting things going.

I do hope that Ionic implement some kind of recommended test framework / guidance, but frankly I can't see that I would move over to it now I have the support of the ng-cli community / base, unless they decide to align themselves very closely with ng-cli.

@masimplo
Copy link
Author

@nicopeeters I faced the same issue when using default import for momentjs. I recommend updating to latest app-scripts and using import * as syntax instead for everything (they are now using webpack so this works again).

@lathonez
Copy link
Owner

@masimakopoulos good to know I was banging my head against this last night.

@masimplo
Copy link
Author

yeap, took me a while to realise what was going on. If you need to identify which particular import is giving you trouble, check the chrome console or switch to phantomjs (which outputs console errors to the terminal)

@cescoferraro
Copy link

my new build with webpack is blazing fast 💯
thanks

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