chore: upgrade to angular 12 #1073
Conversation
This reverts commit 8bf3da5.
Codecov Report
@@ Coverage Diff @@
## main #1073 +/- ##
=======================================
Coverage 85.08% 85.08%
=======================================
Files 822 822
Lines 16924 16928 +4
Branches 2045 2044 -1
=======================================
+ Hits 14399 14404 +5
+ Misses 2494 2493 -1
Partials 31 31
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@itssharmasandeep before merging, please verify ht-ui end to end with real data. Look for any weirdness or any console error. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
not seeing any errors on npm 6 locally. |
what version? I was able to locally repro those build errors with |
|
npm: 6.14.11 node: v15.3.0 |
even with this, i am not seeing any error at install time. What node version are you in? The only error i see is during lint |
I guess it could be the node resolution, but that'd surprise me. I'm able to consistently repro this when running against this branch - and I use node 16.6.1. The error is never at install, it's always at lint - it's basically an artifact, I believe, of how the two versions of npm respect peer dependencies. I was curious so I tried against the main branch, which works fine - the reason, I'm guessing, being that the problematic package that only works in newer npm (ajv-format) wasn't part of the dependency tree until the ng upgrade. |
|
Also, it's continuing to fail because we're not actually doing an install here (at least, that's the first issue) - my oversight, we should update the cache key's prefix (across all jobs), so we don't pull the old result (with the same lock) from cache and instead regenerate. |
This comment has been minimized.
This comment has been minimized.
|
I updated the cached key a bit and checks worked fine, I just changed |
I think making every cache key different in different jobs would not make that big of a difference, I think changing it to start of the workflow inside |
This error might be related to deprecation of |
The lock changed, and it did do a new install - see the last commit where it actually changed ( https://github.com/hypertrace/hypertrace-ui/runs/3372942782 )
More or less - the issue is that the format the parser returns for a deprecation has changed I believe, but the tslint rules are no longer being maintained so the newer typescript is breaking them. I'd say let's either ignore it since we're about to migrate from tslint or remove the deprecation rule (or both!). |
| @@ -21,7 +21,7 @@ jobs: | |||
| uses: actions/cache@v2 | |||
| with: | |||
| path: node_modules | |||
| key: ${{ runner.os }}-node-${{ hashFiles('package-lock.json') }} | |||
| key: ${{ runner.os }}-node-modules-${{ hashFiles('package-lock.json') }} | |||
There was a problem hiding this comment.
this needs to be updated in the publish job, too.
There was a problem hiding this comment.
I saw that and I think we're doing something different in publish job
restoreKeyscontainpackage.jsoninstead ofpackage-lock.jsonin the key value and I think it would always be different right?- we're not checking
cache-hitand every time we're doingnpm ciso cache part seems unused here.
There was a problem hiding this comment.
name: Publish artifacts
on:
# Will only run when release is published.
release:
types:
- created
workflow_dispatch:
jobs:
publish-docker:
runs-on: ubuntu-20.04
steps:
# Set fetch-depth: 0 to fetch commit history and tags for use in version calculation
- name: Check out code
uses: actions/checkout@v2.3.4
with:
fetch-depth: 0
- name: Cache node modules
uses: actions/cache@v2
with:
path: ~/.npm
key: ${{ runner.os }}-node-${{ hashFiles('package.json') }}-${{ hashFiles('package-lock.json') }}
restore-keys: |
${{ runner.os }}-node-${{ hashFiles('package.json') }}-
${{ runner.os }}-node-
- name: NPM Install
run: npm ci
- name: Build
run: npm run build:ci
There was a problem hiding this comment.
whoops - yeah, that one is different but shouldn't be. It will still work, just off fresh node modules each time - so we can come back to that as tech debt.
There was a problem hiding this comment.
should we go ahead and merge this PR?
|
I have tested this with |
itssharmasandeep
changed the title
| @@ -64,7 +64,6 @@ | |||
| "optimization": true, | |||
| "outputHashing": "all", | |||
| "sourceMap": false, | |||
| "extractCss": true, | |||
There was a problem hiding this comment.
Yep. This would fix that warning
This comment has been minimized.
This comment has been minimized.
| @@ -1,3 +1,4 @@ | |||
| @use '~@angular/material' as mat; | |||
There was a problem hiding this comment.
But this can happen separately.
| @@ -329,3 +330,7 @@ $font-family: 'Work Sans', sans-serif; | |||
| @mixin axis-title($color: $gray-9) { | |||
| @include chart-small-regular; | |||
| } | |||
|
|
|||
| $mat-typography: mat.define-typography-config( | |||
There was a problem hiding this comment.
Did this come on its own with migration
There was a problem hiding this comment.
I suggested this from reading the docs - looks like they have some form of migration for it, but not sure exactly what conditions trigger it or if we just skipped it: angular/components#22698
No description provided.