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 9 #11262

Merged
merged 12 commits into from Mar 2, 2020
Merged

Upgrade to Angular 9 #11262

merged 12 commits into from Mar 2, 2020

Conversation

wmarques
Copy link
Contributor

@wmarques wmarques commented Feb 8, 2020

Fix #11255

@wmarques wmarques marked this pull request as ready for review February 17, 2020 12:22
@wmarques
Copy link
Contributor Author

I couldn't use TestBed.inject instead of TestBed.get because we use mock classes instead of the real service so the typing is not working.

Copy link
Member

@ruddell ruddell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the code but did not test the newer changes locally. There is a CI issue related to entity screens. I also restarted the CI due to an npm/cloudflare issue (E429 too many requests) which should be fixed now.

@wmarques
Copy link
Contributor Author

Thanks @ruddell ! Hope that's good, I tested it on my laptop with some entities...

Let's see if the CI is ok 🙏

@pascalgrimaud
Copy link
Member

FYI, the CI failed during the packaging phase here

@wmarques
Copy link
Contributor Author

I need a release after jhipster/ng-jhipster#124 is merged and we should - finally - be good 🎉

@mshima
Copy link
Member

mshima commented Feb 25, 2020

It’s missing tsconfig-aot.json cleanup.

@wmarques
Copy link
Contributor Author

@mshima No I think I removed it, where do you see it ?

@mshima
Copy link
Member

mshima commented Feb 25, 2020

@mshima No I think I removed it, where do you see it ?

I wasn’t clear. The file should be removed from old projects.
https://github.com/jhipster/generator-jhipster/blob/master/generators/cleanup.js

@postpersonality
Copy link

postpersonality commented Feb 27, 2020

Hi. I've applied the changes manually to my existing project and I've noticed lots of warnings like

/src/test/javascript/spec/app/entities/<entity>/<entity>.component.spec.ts is part of the TypeScript compilation but it's unused.
Add only entry points to the 'files' or 'include' properties in your tsconfig.

when doing npm run webpack:build

It was an error in my tsconfig file.

And the second thing I've noticed you have "@angular/common": "9.0.0" in the package.json while there are updates to the 9.0.3.

@wmarques
Copy link
Contributor Author

@postpersonality alright i'll check this out, thanks for reaching up

@postpersonality
Copy link

postpersonality commented Feb 27, 2020

There is also

npm WARN @ng-bootstrap/ng-bootstrap@5.2.1 requires a peer of @angular/common@^8.0.0 but none is installed. You must install peer dependencies yourself.
npm WARN @ng-bootstrap/ng-bootstrap@5.2.1 requires a peer of @angular/core@^8.0.0 but none is installed. You must install peer dependencies yourself.
npm WARN @ng-bootstrap/ng-bootstrap@5.2.1 requires a peer of @angular/forms@^8.0.0 but none is installed. You must install peer dependencies yourself.

during the npm install

I think @ng-bootstrap/ng-bootstrap needs to be updated to 6.0.0

But doing that leads to

npm WARN ng-jhipster@0.12.0 requires a peer of @ng-bootstrap/ng-bootstrap@^5.1.0 but none is installed. You must install peer dependencies yourself.

@wmarques
Copy link
Contributor Author

Looks like 9.0.3 is not ready : angular/angular#35709
I saw this issue on my project so I prefer not upgrading to this version.

@postpersonality
Copy link

postpersonality commented Feb 27, 2020

Looks like 9.0.3 is not ready : angular/angular#35709

Thanks for posting this. Got the same problem with the 9.0.3. The 9.0.2 works fine for me.

@wmarques
Copy link
Contributor Author

@postpersonality I don't have the TS warnings. Did you add
"files": ["src/main/webapp/app/app.main.ts"] in tsconfig.app.json ?

@wmarques
Copy link
Contributor Author

I think this is ready for merge.
cc @kaidohallik @vishal423 if you want to check this 😄

@vishal423
Copy link
Contributor

@wmarques, I will try to have a quick pass by today evening or tomorrow.

Copy link
Contributor

@vishal423 vishal423 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall code looks good. I have provided few minor comments.

generators/client/templates/angular/package.json.ejs Outdated Show resolved Hide resolved
generators/client/templates/angular/package.json.ejs Outdated Show resolved Hide resolved
generators/client/templates/angular/package.json.ejs Outdated Show resolved Hide resolved
@@ -16,6 +16,7 @@
See the License for the specific language governing permissions and
limitations under the License.
-%>
import './polyfills';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any specific reason to move to this approach or can we follow cli generated code to add it to tsconfig.app.json like
"files": ["src/main.ts", "src/polyfills.ts"],

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just thought it reduce entry points, I don't see why we put the polyfills apart...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you confirm that it still produces a different bundle for polyfills? If not, then, it's a deviation from current behavior and also wouldn't recommend that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wmarques, can you confirm the behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't produces a different bundle but the polyfill would still be loaded so for me that's not really important, polyfills are always loaded, we don't do differential loading like angular CLI does.

generators/client/templates/angular/tsconfig.json.ejs Outdated Show resolved Hide resolved
generators/client/templates/angular/tsconfig.json.ejs Outdated Show resolved Hide resolved
@vishal423
Copy link
Contributor

I couldn't use TestBed.inject instead of TestBed.get because we use mock classes instead of the real service so the typing is not working.

In my schematic sample project, I had noticed lint errors/warnings with TestBed.get. Can you check that's not the case here?

Copy link
Contributor

@mraible mraible left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I would like to see Angular 9.0.4 if possible.

generators/client/templates/angular/package.json.ejs Outdated Show resolved Hide resolved
"@angular/localize": "^9.0.0",
"@angular/platform-browser": "9.0.0",
"@angular/platform-browser-dynamic": "9.0.0",
"@angular/router": "9.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Angular 9.0.4 is available https://github.com/angular/angular/releases.

@mraible mraible added $$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ $200 https://www.jhipster.tech/bug-bounties/ labels Feb 29, 2020
@mraible
Copy link
Contributor

mraible commented Feb 29, 2020

I'm assigning a bug bounty to this one because it wasn't that straightforward and it's an important feature for JHipster.

@pascalgrimaud
Copy link
Member

@mraible : there is already a bounty on the issue, but if you think it's not enough, I'm increasing it and remove the bounty on the PR. No need to add bounty on both issue and PR, it can confuse people :p

@pascalgrimaud pascalgrimaud removed $$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ $200 https://www.jhipster.tech/bug-bounties/ labels Mar 1, 2020
@wmarques
Copy link
Contributor Author

wmarques commented Mar 2, 2020

Thanks @mraible and @vishal423, should be good now 😄 (if the CI god is with me 🙏)

@wmarques
Copy link
Contributor Author

wmarques commented Mar 2, 2020

Only one React build failing 😢

@pascalgrimaud pascalgrimaud merged commit 22353e9 into jhipster:master Mar 2, 2020
@wmarques wmarques deleted the ng9 branch March 2, 2020 13:38
@wmarques
Copy link
Contributor Author

wmarques commented Mar 2, 2020

I couldn't use TestBed.inject instead of TestBed.get because we use mock classes instead of the real service so the typing is not working.

In my schematic sample project, I had noticed lint errors/warnings with TestBed.get. Can you check that's not the case here?

Didn't notice anything with npm run lint

@pascalgrimaud pascalgrimaud added this to the 6.8.0 milestone Mar 3, 2020
@wmarques
Copy link
Contributor Author

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.

angular: Upgrade to v9.0
7 participants