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

Upgrade to Angular 6 #7582

Merged
merged 9 commits into from May 16, 2018
Merged

Upgrade to Angular 6 #7582

merged 9 commits into from May 16, 2018

Conversation

wmarques
Copy link
Contributor

@wmarques wmarques commented May 7, 2018

Still WIP, need to check if I correctly migrated the rxjs imports.
I wanted to remove the rxjs-compat dependency but some libs aren't rxjs6 compliant (ngx-webstorage, ngx-infinite-scroll)...

TODO:

  • Migrate angular cli config file to new format
  • Upgrade injection using the new DI injection ?
  • Please make sure the below checklist is followed for Pull Requests.

  • Travis tests are green

  • Tests are added where necessary

  • Documentation is added/updated where necessary

  • Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

@wmarques
Copy link
Contributor Author

wmarques commented May 7, 2018

Seems that we cannot use a custom webpack build with angular-cli... When I try to ng eject an Angular 6 app generated with Angular CLI 6, it tells me that the support for ejecthas been removed 😞

@wmarques
Copy link
Contributor Author

wmarques commented May 8, 2018

I don't know how to be still angular CLI compliant, it always generate the component under a wrong path even if I specifiy src/main/webappas the root src directory in the angular.json file...

@sendilkumarn
Copy link
Member

@wmarques that seems weird, eject is removed in v6, with the idea of using it via angular-cli.json

@sendilkumarn
Copy link
Member

@wmarques lets list the issues that we have currently on this probably in a doc, what say?

@wmarques
Copy link
Contributor Author

@sendilkumarn Yes, strange... If you guys have any idea about adapting the angular.json file to make it work..
Sure, we can merge this once I've resolved the conflict and then I will work on migrating to the new DI injection syntax :)

@wmarques
Copy link
Contributor Author

@sendilkumarn I just rebased, let's wait for Travis and if you are ok with this you can merge

@wmarques wmarques changed the title [WIP] Upgrade to Angular 6 Upgrade to Angular 6 May 10, 2018
@wmarques
Copy link
Contributor Author

Let me rebase again, the travis build fails doesn't seem related to my PR...

@thainan10
Copy link

@wmarques have you tried to follow the steps of this Angular helper website to upgrade the version?

@wmarques
Copy link
Contributor Author

Finally green 🎉

@wmarques
Copy link
Contributor Author

wmarques commented May 15, 2018

I think we can merge this if this is ok for you guys and I'll start migrating on the new DI injection system on a next PR, we also need the ng-jhipster lib to migrate

@pascalgrimaud
Copy link
Member

Congrats @wmarques for the hard work !

@wmarques
Copy link
Contributor Author

Well I had enough time to move to the new Dependency Injection so I merged my work on this PR, let me know if you have any feedbacks

@pascalgrimaud
Copy link
Member

Travis back to red since your last commit, sorry @wmarques

@jdubois
Copy link
Member

jdubois commented May 15, 2018

@pascalgrimaud it's probably my fault, it's corrected now, @wmarques can you rebase?

@pascalgrimaud
Copy link
Member

pascalgrimaud commented May 16, 2018

@wmarques : fyi, there are some conflicts after the merge of your other PR #7634

Update: I used Github interface to resolve the merge conflicts

@jdubois
Copy link
Member

jdubois commented May 16, 2018

This looks all good, let's merge it as this can't be in a separate branch for long (we'll get lots of conflicts)

@jdubois jdubois merged commit f6a544a into jhipster:master May 16, 2018
@sendilkumarn
Copy link
Member

@wmarques great work 👏 It would be awesome to get the stats after this

@jdubois
Copy link
Member

jdubois commented May 16, 2018

@wmarques just one issue: when you do some responsive tests (putting the app in a small window, in fact), the menu doesn't show up :-(
This is probably because of your changes, could you check?

@deepu105
Copy link
Member

@wmarques does the new DI affect lazy loading? Since many services are in root now?

@wmarques
Copy link
Contributor Author

@deepu105 I analyzed the generated bundle and it seems that if the module is lazy loaded Angular puts the injected services in this module in the correct bundle so I didn't see any changes in the output.

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

Successfully merging this pull request may close these issues.

None yet

6 participants