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

Fix hard failure on google maps error #2184

Merged
merged 27 commits into from
Dec 11, 2024
Merged

Fix hard failure on google maps error #2184

merged 27 commits into from
Dec 11, 2024

Conversation

hudson-newey
Copy link
Member

@hudson-newey hudson-newey commented Dec 9, 2024

Fix hard failure on google maps error

Changes

  • Fixes a bug where if Google Maps failed to embed, the entire site would hard-fail
  • Move the embedGoogleMaps.ts helper scripts into a DI service
  • Lazy load google maps to only embed when we need it for the current page (improving performance)
  • Asyncronously embed google maps (embedding google maps is no longer a blocking operating, improving performance and reducing the scope for bugs)
  • Adds loading spinner when Google maps is loading
  • Adds failure state when Google maps fails to load
  • Re-enables map component tests

Problems

Any problems caused by the PR that will need to be address in another PR. For example, these issues may be caused by another branch which will change how this branch functions.

Issues

Fixes: #2175

Final Checklist

  • Assign reviewers if you have permission
  • Assign labels if you have permission
  • Link issues related to PR
  • Ensure project linter is not producing any warnings (npm run lint)
  • Ensure build is passing on all browsers (npm run test:all)
  • Ensure CI build is passing
  • Ensure docker container is passing (docs)

Copy link
Contributor

github-actions bot commented Dec 9, 2024

Size Change: +1 kB (+0.03%)

Total Size: 3.9 MB

Filename Size Change
dist/workbench-client/browser/index.html 4.81 kB -3 B (-0.06%)
dist/workbench-client/browser/main-JGZGEAXM.js 0 B -1.13 MB (removed) 🏆
dist/workbench-client/server/main.js 1.94 MB +487 B (+0.03%)
dist/workbench-client/browser/main-YWRP5WNH.js 1.13 MB +1.13 MB (new file) 🆕
ℹ️ View Unchanged
Filename Size
dist/workbench-client/browser/assets/buffer-builder-processor-BhnxGUx8.js 1.16 kB
dist/workbench-client/browser/assets/environment.json 555 B
dist/workbench-client/browser/assets/high-accuracy-time-processor-BevUJNwo.js 354 B
dist/workbench-client/browser/assets/test-assets/index.html 21 B
dist/workbench-client/browser/chunk-NPDQGTTA.js 1.17 kB
dist/workbench-client/browser/chunk-U5EWAFH4.js 379 kB
dist/workbench-client/browser/manifest.json 147 B
dist/workbench-client/browser/polyfills-C5CKP5CH.js 12.4 kB
dist/workbench-client/browser/styles-ETV6J7SM.css 39.6 kB
dist/workbench-client/server/580.js 390 kB
dist/workbench-client/server/952.js 4.21 kB

compressed-size-action

@hudson-newey hudson-newey self-assigned this Dec 9, 2024
@hudson-newey hudson-newey added the bug Something isn't working label Dec 9, 2024
@hudson-newey hudson-newey marked this pull request as ready for review December 9, 2024 05:59
@hudson-newey hudson-newey requested a review from atruskie December 9, 2024 05:59
@hudson-newey hudson-newey added the architecture Architectural changes to the software label Dec 10, 2024
Copy link
Contributor

github-actions bot commented Dec 10, 2024

Unit Test Results

         6 files           6 suites   8m 55s ⏱️
24 228 tests 23 484 ✔️ 744 💤 0
24 450 runs  23 706 ✔️ 744 💤 0

Results for commit 533b6ee.

♻️ This comment has been updated with latest results.

@@ -72,31 +56,25 @@ export class MapComponent
// Setting to "hybrid" can increase load times and looks like the map is bugged
public mapOptions: MapOptions = { mapTypeId: "satellite" };
public bounds: google.maps.LatLngBounds;
private updateMap: boolean;
private updateMap: boolean = false;
Copy link
Member

Choose a reason for hiding this comment

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

what is the purpose of this field, what does it do?

Copy link
Member Author

Choose a reason for hiding this comment

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

Google maps runs the ngAfterViewChecked method every 100ms, even with DetectChanges.OnPush

This updateMap exists so that we don't need to run this code every 100ms
image

I'm looking into it now because I suspect this is something that we're doing wrong; I wouldn't expect Google to author code that continuously runs ngAfterViewChecked

Copy link
Member

Choose a reason for hiding this comment

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

oh i didn't see that function.

you're right though, seems weird we're hooked into ngAfterViewChecked. Perhaps another hook is more appropriate?

Copy link
Member Author

Choose a reason for hiding this comment

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

Update: ngAfterViewChecked is due to the ViewChild decorators for the GoogleMap, MapInfoWindow, and MapMarker components

src/app/services/maps/maps.service.ts Outdated Show resolved Hide resolved
Copy link
Member

@atruskie atruskie Dec 10, 2024

Choose a reason for hiding this comment

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

ok it seems you've tried to use a promise like a signal.

If you actually want to emit all the various states, perhaps an observeable is the right choice here.

Otherwise, your promise can return values too you know. Like Loaded or Failed.

I don't understand why you need SharedPromise and I don't understand why loadAsync isn't just

return this.loadPromise;

And embedGoogleMaps is an async function that returns a promise. Why is that not just assigned to this.loadPromise ?

This file needs a simplicity pass.

@hudson-newey hudson-newey merged commit fe7e444 into master Dec 11, 2024
14 checks passed
@hudson-newey hudson-newey deleted the google-maps-fix branch December 11, 2024 01:44
hudson-newey added a commit that referenced this pull request Dec 11, 2024
* Fixes a bug where if Google Maps failed to embed, the entire site would hard-fail
* Move the embedGoogleMaps.ts helper scripts into a DI service
* Lazy load google maps to only embed when we need it for the current page (improving performance)
* Asyncronously embed google maps (embedding google maps is no longer a blocking operating, improving performance and reducing the scope for bugs)
* Adds loading spinner when Google maps is loading
* Adds failure state when Google maps fails to load
* Re-enables map component tests

Fixes: #2175
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture Architectural changes to the software bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adblockers blocking Google Maps will cause the client to hard-fail
2 participants