Closed leifjones closed 3 years ago
@mrsegen I see this is marked as a draft -- what work is outstanding to productize your updates?
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
I hope to report back tomorrow on what the local changes were that I wasn't able to push
@NateRadebaugh I can't figure out how to push the local changes due to the checks in place. But the spirit of this was:
ng update @angular/cli @angular/core -C
"aot": true
under options in angular.jsonIf you can support (or advise on) upgrading this package to be Ivy-compatible, that'd be very helpful.
Based on this link, it looks like either your versions are mismatched or the aot
flag is in the wrong spot:
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 ๐
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?
is there any update available? But project seems to be fairly dead?
@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'
@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).]
@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.
@cshouts-tasc Thanks that seemed to do it!
Hey @mrsegen Thanks for your PR ! ๐ Could you rebase it to let @moribvndvs merge it ? ๐
Cool! I'm looking forward to this as well. Let me know when a new npm package with this is available...
@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.
@worksoncloud @SuperITMan Updated as suggested. Also, I incremented the version to 12
@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 ๐
@SuperITMan I ran those commands (accepting incoming changes on the conflicts) and force pushed.
@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",
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. ๐
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
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?
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.)
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.
@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
Thanks for the info and all the hard work!
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")
What is the current behavior? (You can also link to an open issue here)
What is the new behavior?
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ... (Not known)
Other information: