Feature/connect - Githubissues

EyeSeeTea / FIRE-WiFiCalling

WiFi Calling App to be used with LibreServer

GNU General Public License v3.0

5 stars 1 forks source link

Feature/connect #38

Closed MurhafSousli closed 7 years ago

MurhafSousli commented 7 years ago

Main changes:

Upgrade the store to ngrx v4.0.0 (new syntax/folder structure)
Implement FireHttp to add auth headers for all requests
Implement AuthGuard class to secure any page
Sign in using FIRE-MockServer

Side changes:

Move auth page (login/register) into auth folder.
Remove Core component (move the code in core back to app.component).
Add SecureStorage plugin to store and retrieve authentication keys from user's phone. (to be discussed)
Add documentations.
Tiny fixes here and there.
Update some packages.

MurhafSousli commented 7 years ago

@adrianq PR is ready to be merged

adrianq commented 7 years ago

@MurhafSousli I am getting this error when running ionic serve. Can you please have a lot at it and let me know? I deleted node_modules folder, npm install and ionic serve. screenshot from 2017-08-19 12-56-46

MurhafSousli commented 7 years ago

@adrianq I just double checked it now (got a fresh clone and npm install). It is working without getting this error. but sometimes npm gets the old package files instead of installing the selected version. maybe you need to clear npm cache!

I can see from the screenshot that the installed StoreModule is the older version therefore it is causing the error, Can you try a fresh clone instead?

adrianq commented 7 years ago

Ok, will do and let you know. Thank you for looking into this that quick!

adrianq commented 7 years ago

@MurhafSousli I couldn't make it work. I have tried everything: fresh git clone, clean the cache, delete .npm folder, update store to 4.0.3, read a few interesting threads for a Sunday morning (https://github.com/ngrx/platform/issues/189, https://github.com/ngrx/platform/issues/264)... Finally, the bit of code I have just committed seems to do the trick. At least I can run the server and log-in. Please give it a try yourself and let me know if it is working for you...

adrianq commented 7 years ago

btw, I am also getting this warning when running ionic serve in case you can have a look at it too: screenshot from 2017-08-20 11-31-44 Thanks!

MurhafSousli commented 7 years ago

@adrianq For the warning, if you look at the file auth.ts

We are actually using the import fromAuth, so the warning is false and the linter is not detecting the usage. Most probably it is because we are using import * as fromAuth and then we are only using its inner state fromAuth.State.

This can be solved by importing the interface directly instead. but this is the way ngrx/platform do in all their examples, check their login-page

I think It is better to have something like Travis CI or GitLab to lint and run a build on every push even if it is passing the errors. we will still be able to look at the online terminal

Do you want to make any changes to this PR?

adrianq commented 7 years ago

Ok, thanks @MurhafSousli. I saw that it was being used and didn't want to change it. What do you think about my previous comment and commit? It is strange that you don't see that error but if my change doesn't break anything for you I can keep reviewing the PR and merge it.

... and yes I want to have travis running lint. I even made an attempt. We can talk about it tomorrow...

MurhafSousli commented 7 years ago

I just opened a new issue regards to this linting warning on ngrx/example, let's see what they say about it

@adrianq Yes, let's keep the recent commit if it fixes the build, It won't break anything on my side. btw we have the same interface used in the later PRs.