Angular 12 - Githubissues

leifjones commented 3 years ago

Please check if the PR fulfills these requirements

[x] The commit message follows our guidelines: https://github.com/HackedByChinese/ng2-idle/blob/master/CONTRIBUTING.md#pr
[ ] Tests for the changes have been added (for bug fixes / features) I'm open to suggestions on what to check for here.
[x] Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[x] Other... Please describe: Angular 12 upgrade & enable Ivy compilation 🤞🏻

What is the current behavior? (You can also link to an open issue here)

Doesn't appear to be compatible with projects after upgrading to Angular 10 if Ivy is enabled.
Over 1,000 vulnerabilities

What is the new behavior?

It is hoped that this will enable Ivy compilation.
28 vulnerabilities require semver-major dependency updates.
5 vulnerabilities require manual review

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[X] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ... (Not known)

Other information:

coveralls commented 3 years ago

Coverage remained the same at 100.0% when pulling 3af45c6883a4cb68337c62baa962e853948c65fc on mrsegen:ivy into 086a353923d40d9f9dd72c1e48ccdb90d36feaa4 on moribvndvs:master.

NateRadebaugh commented 3 years ago

@mrsegen I see this is marked as a draft -- what work is outstanding to productize your updates?

leifjones commented 3 years ago

Hi @NateRadebaugh I haven't worked more on this since making that update. IIRC, the blocker was something about the husky checks not accepting aot: true.

I comments as I did above with the hope that someone more familiar with this tool could either pick it up or point me in a direction

leifjones commented 3 years ago

I hope to report back tomorrow on what the local changes were that I wasn't able to push

leifjones commented 3 years ago

@NateRadebaugh I can't figure out how to push the local changes due to the checks in place. But the spirit of this was:

Update via ng update @angular/cli @angular/core -C
Add "aot": true under options in angular.json

If you can support (or advise on) upgrading this package to be Ivy-compatible, that'd be very helpful.

NateRadebaugh commented 3 years ago

Based on this link, it looks like either your versions are mismatched or the aot flag is in the wrong spot:

https://github.com/angular/angular-cli/issues/11479

Maybe the aot flag isn't needed anymore since everything is aot in ivy?

Note, I have no affiliation with this project, just a concerned citizen like you 🙃

julius-numo commented 3 years ago

According to the docs building and publishing Ivy libs to npm is discouraged: https://angular.io/guide/creating-libraries#publishing-your-library

For now, it is not recommended to publish Ivy libraries to NPM because Ivy generated code is not backward compatible with View Engine, so apps using View Engine will not be able to consume them. Furthermore, the internal Ivy instructions are not yet stable, which can potentially break consumers using a different Angular version from the one used to build the library.

Could this get merged without that but with updated deps and ng 11 compatability?

StefanKoenigMUC commented 3 years ago

is there any update available? But project seems to be fairly dead?

leifjones commented 3 years ago

@cshouts-tasc I made your suggested change locally, but I'm blocked form being able to push the change and getting this output:

husky > pre-push (node v14.15.3)

> ng-idle@10.0.0 lint C:\[path]\GitHub\ng2-idle
> ng lint core && ng lint keepalive

TSLint's support is discontinued and we're deprecating its support in Angular CLI.
Linting "core"...
To opt-in using the community driven ESLint builder, see: https://github.com/angular-eslint/angular-eslint#migrating-an-angular-cli-project-from-codelyzer-and-tslint.
All files pass linting.
TSLint's support is discontinued and we're deprecating its support in Angular CLI.
To opt-in using the community driven ESLint builder, see: https://github.com/angular-eslint/angular-eslint#migrating-an-angular-cli-project-from-codelyzer-and-tslint.
Linting "keepalive"...
All files pass linting.

> ng-idle@10.0.0 pretest C:\other-dev\GitHub\ng2-idle
> npm run build

> ng-idle@10.0.0 prebuild C:\other-dev\GitHub\ng2-idle
> npm run clean:dist

> ng-idle@10.0.0 clean:dist C:\other-dev\GitHub\ng2-idle
> rimraf dist

> ng-idle@10.0.0 build C:\other-dev\GitHub\ng2-idle
> ng build core --aot true && ng build keepalive --aot true

Unknown option: '--aot'
Unknown option: 'true'
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! ng-idle@10.0.0 build: `ng build core --aot true && ng build keepalive --aot true`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the ng-idle@10.0.0 build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\[usernam]\AppData\Roaming\npm-cache\_logs\2021-06-17T17_36_34_271Z-debug.log
npm ERR! Test failed.  See above for more details.
husky > pre-push hook failed (add --no-verify to bypass)
error: failed to push some refs to 'https://github.com/mrsegen/ng2-idle.git'

leifjones commented 3 years ago

@julius-numo Could you clarify what specific change you're suggesting? [I'm not a developer on the project - just someone trying to upgrade an application to Angular 11 (now 12).]

cshouts-tasc commented 3 years ago

@mrsegen Libraries use a different builder (ng-packagr) which only accepts a subset of the options of the standard builder (build-angular). AOT is not one of those options, so using the CLI argument --aot is not going to work here.

Angular issue to improve the docs for the build options available when building a library.

leifjones commented 3 years ago

@cshouts-tasc Thanks that seemed to do it!

SuperITMan commented 2 years ago

Hey @mrsegen Thanks for your PR ! 👍 Could you rebase it to let @moribvndvs merge it ? 😊

HarelM commented 2 years ago

Cool! I'm looking forward to this as well. Let me know when a new npm package with this is available...

leifjones commented 2 years ago

@SuperITMan @moribvndvs Thanks! Done! There's a lot of commits appearing above due to pull/push with my local. But, to simplify review, note that these are the only changes between the commit approved above and the current latest. (I addressed ~40 vulnerabilities by upgrading lerna.)

I imagine that we'd want to bump the version to 12, but I'll let you all decide those specifics.

leifjones commented 2 years ago

@worksoncloud @SuperITMan Updated as suggested. Also, I incremented the version to 12

SuperITMan commented 2 years ago

@mrsegen I see you want to merge 69 commits 😮

Could you rebase your commits on top of master branch ?

git remote update
git rebase upstream/master # with upstream = https://github.com/moribvndvs/ng2-idle.git

Regarding the CHANGELOG, I think is generated automatically by @commitlint/config-conventional and you should not edit the file manually. Commitlint uses your commit messages to update the CHANGELOG.

The same thing regarding the package version. The version should be updated by Lerna when we'll release the new version 12.

I didn't notice these details before but I think they should be solved before merging this PR.

Thanks for your work 👍

leifjones commented 2 years ago

@SuperITMan I ran those commands (accepting incoming changes on the conflicts) and force pushed.

SuperITMan commented 2 years ago

@mrsegen The issue I see is that you have multiple duplicated commits 😕

For instance: 5bf20e036a80fd85554d9e48826d963cbb784734 and e37f20dc037e2459aec8a55f1aed7c5f6efdcd92 both change "package.json" file:

"@commitlint/cli": "^8.3.5", --> "@commitlint/cli": "^11.0.0",

leifjones commented 2 years ago

Yeah, I suspect it was because I pulled them pushed on Sunday/Monday instead of force pushing.

If a new branch is the only way, no promises on when I'll get to it.

Lesson learned that force pushing is better. I'm more familiar with merging than rebasing. I suppose I could try squashing it all in a new separate branch. If anyone has a concrete suggestion, let me know. Also, if someone else is interested to replace this PR, you have my blessing. 😄

SuperITMan commented 2 years ago

Hey @mrsegen

I did my best to do a good rebase. Please feel free to take my branch: https://github.com/superitman/ng2-idle/tree/feature/upgrade-ivy-angular-12, check it seems correct to you and force push it here 😊 Like this, we keep the comments in a single PR

moribvndvs commented 2 years ago

As long as the changes are good, I could just squash it when I merge rather than fart around with force pushing and the likes.

Another question I have is about Angular 11 support. Do we just skip over that guy, modify this so it can cover 11 and 12, or patch in 11 support via another branch + tag?

leifjones commented 2 years ago

I appreciate the effort on the cleaner branch! It'd likely be another several days before I get to it. But I'm also fine with the squash merge.

I can go either way on 9+ vs 9-12.

If change is desired (on either counts), feel free to edit. (I think I enabled contributors to edit the branch.)

NgRepoMan commented 2 years ago

What is the status of this PR? We have a bunch of projects waiting to be upgraded to Ang 12 and this dependency is the last one on our list before we can start.

leifjones commented 2 years ago

@SuperITMan, I reset the branch to your and force pushed those changes here. Thank you!

@moribvndvs, it should be good to go now.

Thanks, too, to @cshouts-tasc, @julius-numo, and @NateRadebaugh for your input.

cc: @NgRepoMan, @StefanKoenigMUC, and @HarelM that this might be released soon

HarelM commented 2 years ago

Thanks for the info and all the hard work!

moribvndvs / ng2-idle

Angular 12 #151