Closed chrismclarke closed 5 months ago
Visit the preview URL for this PR (updated for commit 07194ed):
https://idems-debug--pr2157-core-ng-17-t4chc38q.web.app
(expires Wed, 13 Mar 2024 15:16:21 GMT)
🔥 via Firebase Hosting GitHub Action 🌎
Sign: f2bfd21fc73bf25db0e74968614b72bee30e3476
Visual Test Summary new : 0 different : 334 same : 0
Largest Differences 1 | 47.9% | comp_form 2 | 30.7% | feature_tile 3 | 26.1% | comp_pdf 4 | 23.4% | debug_pop_up_return 5 | 8.9 % | feature_number_selector 6 | 8.7 % | example_widget_nesting_audio 7 | 8.3 % | comp_square_button 8 | 8.1 % | example_widget_audio_def 9 | 5.7 % | debug_tile_text 10 | 5.2 % | debug_go_to_url
Download Link https://nightly.link/IDEMSInternational/parenting-app-ui/actions/runs/8082629090
Run Details https://github.com/IDEMSInternational/parenting-app-ui/actions/runs/8082629090
@jfmcquade Would you be able to take a first pass and the review notes to check the various potential issues before we pass for more thorough functional test?
Also feel free if you either want to take on or create a follow-up issue for the core updates mentioned and not done (e.g. swiper, marked, introjs)
Other issues The video below shows 2 distinct issues relating to template loading and navigation The list of templates available at /template no longer auto updates based on search, you need to press enter for the search query to have an effect
Appears to be regression introduced by ionic (flagged in https://github.com/ionic-team/ionic-framework/blob/main/BREAKING.md#searchbar), where change event only fires after comitted change (i.e. user de-focuses input box). I've replaced the ionChange
event binding to use ionInput
which fires on input instead b549c66. I also checked the codebase and only instance of ion-searchbar I could find, and only other calls of ionChange are for elements without input so I expect should work as expected (e.g. radio buttons, checkboxes, where click event should commit the change).
It would be good to skim the rest of the ionic changelog to catch any other likely issues
Sometimes, selecting a particular template from the list does not trigger the template to load
I can't recreate, so let me know if the previous fix has resolved the issue
comp_audio Separate issue detected when checking - appears the seek bar is mis-aligned from player. Is this something we've knowingly changed @jfmcquade ?
When comparing to active debug deployment seems like the sizing has changed a little, throwing the seek bar off by 19px or so (hardcoded top
offset if we do want to change it)
rxdb v13->v14 comp_data_items throws the following error
On closer review this warning is currently thrown on master, so not a regression introduced. The issue appears to be related to multiple function calls all to initialise a specific db collection in parallel (3 requests all to populate db docs, the first passes and next 2 fail). So not a breaking issue, but I've gone ahead and applied a fix to this branch (as I had done that before realising the issue was in master) 993b06f
ng2-nouislider v1->v2
- comp_slider is broken
![]()
- when exiting a template with a slider component, I get the following error
![]()
After banging my head against a wall looking into the issue with comp_slider
, it seems to have started reliably working for me.
What appeared to fix the issue was manually reverting the version number of nouislider
in package.json
to ^14.0.0
and running yarn install
, before manually changing it back to ^15.7.1
and running yarn install
again. Before I did this, I was testing by alternately checking out master
and core/ng-17
and running yarn install each time
, to test the functionality on the respective branches. Every time I checked out core/ng-17
ran yarn start
(after yarn install
), the nouislider
component wouldn't render any content inside it. Now, after manipulating package.json
, it seems that it reliably does render correctly.
If this was the source of the fix, then I can only assume it was some kind of caching error where the correct package version was not actually being used? Although yarn should handle such issues. I may have mis-diagnosed the solution, but there have been no changes to local files tracked by git.
Appears to be regression introduced by ionic (flagged in https://github.com/ionic-team/ionic-framework/blob/main/BREAKING.md#searchbar), where change event only fires after comitted change (i.e. user de-focuses input box). I've replaced the
ionChange
event binding to useionInput
which fires on input instead b549c66. I also checked the codebase and only instance of ion-searchbar I could find, and only other calls of ionChange are for elements without input so I expect should work as expected (e.g. radio buttons, checkboxes, where click event should commit the change).
I have scanned through the ionic changelog looking for other potential issues. The only thing that stands out is, what you've identified here: various other components now also do not emit ionChange
when they used to. For example, for ion-checkbox
:
ionChange
is no longer emitted when thechecked
property ofion-checkbox
is modified externally.
So if we change the value of the checkbox component from elsewhere in the template, ionChange
will not be triggered and any actions set to trigger on the the changed
trigger on the checkbox will not be triggered. As far as I can tell, this isn't something we currently do, but is worth being aware of.
An equivalent change has been made to datetime
, ion-range
, ion-input
, ion-select
, ion-textarea
, ion-radio-group
– I've looked into how we use each (if at all) and I don't believe the change affects us.
One other component which has this change and for which we do use ionChange
is ion-toggle
. We don't use ionChange
on ion-toggle
in the toggle-bar
component, but we do use it in the debug-toggle
component. As far as I can tell, the debug toggle is the only way to set debug mode, so I don't foresee any issues.
What appeared to fix the issue was manually reverting the version number of
nouislider
inpackage.json
to^14.0.0
and runningyarn install
, before manually changing it back to^15.7.1
and runningyarn install
again. Before I did this, I was testing by alternately checking outmaster
andcore/ng-17
and runningyarn install each time
, to test the functionality on the respective branches. Every time I checked outcore/ng-17
ranyarn start
(afteryarn install
), thenouislider
component wouldn't render any content inside it. Now, after manipulatingpackage.json
, it seems that it reliably does render correctly. If this was the source of the fix, then I can only assume it was some kind of caching error where the correct package version was not actually being used? Although yarn should handle such issues. I may have mis-diagnosed the solution, but there have been no changes to local files tracked by git.
Super weird and imagine a total headache, but good to know things working now at least. So in terms of outstanding issues is it just comp_audio
now as far as we're aware?
So in terms of outstanding issues is it just
comp_audio
now as far as we're aware?
I think that's right, pending further functional testing. There is an issue which proposes a redesign of the audio player, #2147, and I don't think that we need to keep the existing design, so it may be that the most efficient thing to do is to prioritise that issue. I can confirm tomorrow
Thanks for checking all these @esmeetewinkel
@jfmcquade
- A line appears below a text box when you click it to type text
Looking through changelogs I can't actually find when/why this has started being applied, but it comes from an .input-highlight
class which we may want to consider overriding (e.g. adding display:none
) if we do not want this behaviour on any components that use an ion-input. Are you able to fix this?
- Checkbox "unticked" background colour changed to light grey
Looks like this should have been caught from the Breaking Changes doc. Would you be able to fix and take another pass for any similar potential issues?
- Toggle bar visually broken (actions are working fine): in FALSE state the ball doesn't appear entirely on the LHS of the control, and the TRUE state doesn't work at all. The same problem (but reversed) happens when the starting value of the toggle was TRUE. Also it seems that our custom toggle styles (with tick and cross icons) are no longer interpreted.
I'm guessing this is related to the ionChange updates described in breaking changes, but very weird given that we don't actually use that method (guessing there's some knock-on for how it handles click events). From what I can tell the click event is firing twice, so guessing there's some deeper changes to underlying elements used. I've fixed by adding an additional event block in 2c18594
- On the debug template
comp_odk_form
the back button in the top left corner appears tiny This is a much more awkward one, it appears a lot of the ionic components have updated styles to move away from fixed units (e.g. 48px) to relative ones (e.g. 3rem). Throughout most of the app this has no noticeable effect (as default em/rem tends to be 16px), but the odk component interferes with this. The changes are somewhat out of control, as the odk component has to import external stylesheets (where I'm assuming the changes are coming from), and have no view encapsulation to display correctly (hence why changes are able to bleed out).
So I'll need to take a deeper dive to try figure out a plan for this. I'm not sure if we're actively using this component anywhere, it might be worth temporarily disabling whilst we get the rest of changes merged in as the impact persists across all templates viewed after the odk component has been loaded
7. On the debug template
comp_odk_form
the back button in the top left corner appears tiny This is a much more awkward one, it appears a lot of the ionic components have updated styles to move away from fixed units (e.g. 48px) to relative ones (e.g. 3rem). Throughout most of the app this has no noticeable effect (as default em/rem tends to be 16px), but the odk component interferes with this. The changes are somewhat out of control, as the odk component has to import external stylesheets (where I'm assuming the changes are coming from), and have no view encapsulation to display correctly (hence why changes are able to bleed out).So I'll need to take a deeper dive to try figure out a plan for this. I'm not sure if we're actively using this component anywhere, it might be worth temporarily disabling whilst we get the rest of changes merged in as the impact persists across all templates viewed after the odk component has been loaded
I can confirm we're not actively using the ODK component in any deployment at the moment.
ODK form styling should fixed with 582452b.
I'm not entirely sure why they were originally setup the way they were, but now should be working as expected
Thanks @esmeetewinkel for bumping this. I'll have a look this week and try to resolve what I can (including merge conflicts)
To summarise from call:
I've performed the planned updates, and added a few minor fixes. I believe we've now addressed all of the sisues that came up in the PR comments. @chrismclarke, you can see my changes since your last commit here.
Updated @ionic/angular
7.5.6 -> 7.7.3.
I read through the release notes here, I couldn't find any changes that I'd expect to cause any issues, except for some updates to the ion-toggle
component.
Our toggle component (which uses ion-toggle) uses a now legacy syntax. I've made an issue to update this: #2212.
Updated Angular 17.0.x -> 17.2.2.
Running ng update
suggested the following package updates:
@angular-eslint/schematics 17.1.0 -> 17.2.1
@angular/cli 17.0.10 -> 17.2.1
@angular/core 17.0.9 -> 17.2.2
@angular/fire 17.0.0-next.0 -> 17.0.1
@ngx-matomo/tracker 1.3.6 -> 4.1.2
I've consulted the Angular changelog and didn't find any expected problems.
When running ng update @angular/core
, Angular CLI flagged that @ngx-matomo/router
and @ngx-matomo/tracker
required updating (^1.3.3 -> ^4.1.2) to maintain compatibility with @angular/core
. After applying these updates, I discovered these packages have been replaced with ngx-matomo-client, with the migration already handled as part of this PR. So I have removed the unused @ngx-matomo/router
and @ngx-matomo/tracker
packages.
It would be good to test matomo for any issues, presumably after merging?
Likewise @sentry/angular
also required an update, changing the package from @sentry/angular to @sentry/angular-ivy docs here. The API appears to be the same, none of the method calls (solely in src/app/shared/services/error-handler/error-handler.service.ts
) have any issues flagged within the code.
Thanks @jfmcquade - I appreciate that I'm the one who opened the PR so can't give a review, but gone through all your changes and looks good to me. I've also added a note #2211 to update sentry import there accordingly.
Agreed we can check on sentry and matomo post merge, and thanks for adding the follow-up issue for ion-toggle
Thanks, @esmeetewinkel.
I've pushed changes to fix the first 3 issues you mention:
Acquiring an exclusive Navigator LockManager failed
Regarding the yellow warnings logged in your final screenshot:
I'm only getting the RxDB dev-mode warning
one:
Are there any steps you take that cause those warnings to appear, e.g. navigating to a particular template? I've definitely seen the warnings related to ODK before, I think we just have to accept them for now as they are related to the ODK package we're using.
As far as the 3 warnings regarding "CommonJS or AMD dependencies" are concerned, @chrismclarke could you advise? Should I be adding these packages to allowedCommonJsDependencies
in angular.json
? Or should we be looking to move away from these packages altogether?
Regarding the yellow warnings logged in your final screenshot: I'm only getting the
RxDB dev-mode warning
one:
During compilation I expect you probably see a lot of warnings coming from ./src/app/shared/components/template/components/odk-form
, but these won't always appear in the console depending whether it is first load or a reload action.
There's not a huge amount we can do about all the odk warnings at this stage. It's 3rd party code that we've essentially copy-pasted into the repo, so not going to conform to our syntax guidelines. The best solution would probably be to get it all running as part of a standalone repo and just importing the compiled version from there, that's very far down a list of priorities I already have on a separate project I'm working on (PICSA), so I think possibly not worth too much time investment from here.
As far as the 3 warnings regarding "CommonJS or AMD dependencies" are concerned, @chrismclarke could you advise? Should I be adding these packages to
allowedCommonJsDependencies
inangular.json
? Or should we be looking to move away from these packages altogether?
Yes you simply need to add the names of the allowedCommonJsDependencies
in the angular.json
file. CommonJS is never going away, some packages will never migrate to ESM and that's not a fundamental issue (yes, harder for compilers to optimise, but a well-crafted commonJS package will still be better than a poorly assembled ESM one), so I think a bit of overreach on the angular warnings that can be ignored (given the common actions are to either ignore or add to the list of warnings to suppress).
@jfmcquade
Judging by https://github.com/angular/angular-cli/pull/26047
it might now just be possible to set '*'
in the list of allowedCommonJsDependencies
to surpress all warnings.
Should have gone live in @angular-devkit/build-angular 17.1.0, which I think we have upgraded past as part of this PR
Thanks @chrismclarke. The new wildcard option does seem to work for suppressing the CommonJS warning messages.
@esmeetewinkel I think this should be ready to merge once you're satisfied I've addressed the points in your last comment
PR Checklist
TODO (follow-ups)
Breaking Changes
Description
Various core depencies updated. Major version bumps for:
Web
Not Updated Major updates, but independent to angular so recommend follow-up
Android
Not Updated
Dev Notes
Can use signals, standalone components and template control syntax. At this point not auto-migrating, more opt-in. https://blog.angular.io/angular-v16-is-here-4d7a28ec680d https://blog.angular.io/introducing-angular-v17-4d7033312e4b
I've been through Angular update guuide, Items that stand out for me are listed below (have attempted/assumed fixes for all):
[x] Update your code to use ViewContainerRef.createComponent without the factory resolver. ComponentFactoryResolver has been removed from Router APIs.
[x] Due to the removal of the Angular Compatibility Compiler (ngcc) in v16, projects on v16 and later no longer support View Engine libraries.
[x] entryComponents is no longer available and any reference to it can be removed from the @NgModule and @Component public APIs.
[x] Make sure that you are using a supported version of node.js before you upgrade your application. Angular v17 supports node.js versions: v18.13.0 and newer
In future may want to enforce specific code styles/practices, but recommend only to do so if we have automated migration and linting first
I removed the ngx-color dependency which used to be used for a color-picker as part of custom theme editor, but appears to be no longer in use
I also did my best to look through changelogs on all major package updates, and either address where quickly possible or note in review notes as something to check
Author Notes
If running locally it is now expected that Node v18+ in use. This isn't strictly enforced (so not marking as breaking change), but I've updated the readme accordingly.
If receiving errors when installing it might be a case that the local
node_modules
folder requires deletion first (I had to due tohttps://github.com/angular/angular-cli/issues/25377
, but hopefully the updated yarn.lock file should mean others don't)Review Notes
A lot of core updates here, so really want to check all core functionality including:
Template loading/navigation more complex templates load/navigate as expected as a few changes made to router handling
Template component UI potential breaking changes, not yet addressed from https://github.com/ionic-team/ionic-framework/blob/main/BREAKING.md#version-7x. E.g. can see in test visual some button sizing changes
Firebase (login)
Matomo analytics
PDF viewer Haven't checked changelog, but comp-pdf visual test appears to be showing different pages so could be a regression
Compodoc generated docs Untested but assume compatible with ng 17
Lottie animations
Native functionality ( login) Mostly to ensure interoperability with dep updates (e.g. firebase). I
noui slider component
rxdb functionality (reactive memory, used in dynamic data service and data-items component/remote asset service). Have fixed imports/method names according to https://rxdb.info/releases/14.0.0.html but harder to check deeper immutability and object return type changes. Additionally there are data migrations required which I've implemented, but would be good to test knock-on (appears to be a
${deploymentName}_user
collection, but not sure if actually used) - ideally would be good to have a local state that uses the persistedMemory adapter, and just check things still appear as expected when changing to pr branch. From what I can tell the changes should all be internal (changes to metadata fields like _rev, which used to be automatically populated to schema json but now no longer present; so assume implied somewhere else in rxdb codebase)Markdown rendering (only mention in changelog is copy-tex css which I don't see referenced anywhere so hopefully fine)
Git Issues
Closes #
Screenshots/Videos
If useful, provide screenshot or capture to highlight main changes