map not loading when online #26998
map not loading when online #26998
Conversation
| @@ -3,6 +3,10 @@ import {AppRegistry} from 'react-native'; | |||
| // This is a polyfill for InternetExplorer to support the modern KeyboardEvent.key and KeyboardEvent.code instead of KeyboardEvent.keyCode | |||
| import 'shim-keyboard-event-key'; | |||
|
|
|||
| // load all chunk file map when online | |||
| import 'react-map-gl'; | |||
There was a problem hiding this comment.
We don't have to do it here, should have a better way to load it. : )
There was a problem hiding this comment.
@ntdiary do you have any suggestion here where we need load. when I am checking https://github.com/Expensify/App/blob/d89ca7043b9bc923c0599836a3571bb15d40ed7c/src/components/MapView/MapView.web.tsx#L8C36-L8C48 loading this but when we open distance page at the time only components mounted so it's loading all necessary chunk file when offline this is ignored.
Note: This will be needed only for the web and desktop. so I added on the platform setup this will call initially and load all chunk files. like shim-keyboard-event-key
There was a problem hiding this comment.
@ntdiary could you check this
There was a problem hiding this comment.
I understand your point, but we already have MapView.web.tsx, we can also do it this way (available on both web and desktop):
diff --git a/src/components/MapView/MapView.web.tsx b/src/components/MapView/MapView.web.tsx
--- a/src/components/MapView/MapView.web.tsx (revision f896f71a1ce00320a2ba029e44396d4e29b88223)
+++ b/src/components/MapView/MapView.web.tsx (date 1694452383426)
@@ -14,6 +14,7 @@
import Direction from './Direction';
import {MapViewHandle, MapViewProps} from './MapViewTypes';
+import MapboxGl from 'mapbox-gl';
import 'mapbox-gl/dist/mapbox-gl.css';
const MapView = forwardRef<MapViewHandle, MapViewProps>(
@@ -65,6 +66,7 @@
{...responder.panHandlers}
>
<Map
+ mapLib={MapboxGl}
ref={setRef}
mapboxAccessToken={accessToken}
initialViewState={{
I'm not too sure if import() is allowed, if so, we can also do it this way:
diff --git a/src/components/MapView/MapView.web.tsx b/src/components/MapView/MapView.web.tsx
--- a/src/components/MapView/MapView.web.tsx (revision f896f71a1ce00320a2ba029e44396d4e29b88223)
+++ b/src/components/MapView/MapView.web.tsx (date 1694452383426)
@@ -65,6 +65,7 @@
{...responder.panHandlers}
>
<Map
+ mapLib={import('mapbox-gl')}
ref={setRef}
mapboxAccessToken={accessToken}
initialViewState={{
Additionally, let's wait for @hayata-suenaga's opinion to see if we should continue with this PR. 🙂
There was a problem hiding this comment.
@ntdiary MapView.web.tsx already we have the import statement. the actual problem is MapView.web.tsx will trigger (mount) only when you open a distance request page like a token api trigger. before open up the distance request page your offline means map file(chunk) is not loading.
There was a problem hiding this comment.
@pradeepmdk, in the first approach, mapbox-gl will also be bundled into the vendors-xxx.js (instead of being a separate js file), so subsequently it can be safely loaded. In the second approach, because there is a preceding isOffline condition, mapbox-gl can also be safely loaded when the device comes back online. If you find it hard to believe, please test it out first, and feel free to let me know if you have any other questions.
App/src/components/DistanceRequest.js
Line 238 in d6e26af
There was a problem hiding this comment.
@ntdiary this is working mapLib={import('mapbox-gl')} let me update PR
|
I'm sorry I'm juggling several issues in addition to a different project could you tell me what chunk files are and why we need to touch file import statements in this PR? |
Untitled.mov@hayata-suenaga when offline the first time opening the distance request and after that when try open multiple times map doesn't load. |
 @hayata-suenaga, our Map component comes from the react-map-gl package, it uses the const mapLib = import('mapbox-gl'); dynamic import statement to load the mapbox-gl package.This causes the mapbox-gl package to be built as a separate js file (i.e. 158-70d9a0576645807b49ed.bundle.js), and the above code only executes once.So once the distance request page is first opened when the device is offline, this js file will fail to load properly, then afterwards even if the device comes back online the Map component will be unable to render (because mapLib promise is already in a rejected state).
To solve this problem, we either need to bundle the mapbox-gl-trunk.mp4 |
|
thank you so much for the explanation. could you explain more about these two options?
|
@hayata-suenaga, it's my pleasure.
This approach is module (or static) import, diff --git a/src/components/MapView/MapView.web.tsx b/src/components/MapView/MapView.web.tsx
import Map, {MapRef, Marker} from 'react-map-gl';
+import MapboxGl from 'mapbox-gl';
import responder from './responder';
import utils from './utils';
@@ -65,6 +66,7 @@ const MapView = forwardRef<MapViewHandle, MapViewProps>(
{...responder.panHandlers}
>
<Map
+ mapLib={MapboxGl}
ref={setRef}
mapboxAccessToken={accessToken}module-import.mp4
This approach is dynamic import (will still enable bundle splitting). diff --git a/src/components/MapView/MapView.web.tsx b/src/components/MapView/MapView.web.tsx
@@ -65,6 +65,7 @@ const MapView = forwardRef<MapViewHandle, MapViewProps>(
{...responder.panHandlers}
>
<Map
+ mapLib={import('mapbox-gl')}
ref={setRef}
mapboxAccessToken={accessToken}
initialViewState={{dynamic-import.mp4 |
|
ah I see that made thing more clear @ntdiary which do you think is the best option? I think dynamic import seems better to me? |
|
@hayata-suenaga , eh, Besides the |
|
@ntdiary @hayata-suenaga dynamic import is working fine in our project. I already updated the PR |
Is there a way to suppress the error log? why do we get the error log? (sorry for asking a lot of questions)
I thought we're using dynamic imports in some of the screens (we only load/fetch components code when navigated to them?) |
@hayata-suenaga, using By the way, don't worry, I think having a clear and comprehensive understanding is more helpful for the quality of our project. 🙂
{
'./src/pages/iou/DistanceRequestPage.js': () => {
eval('<DistanceRequestPage xxx />');
},
}For example, when we open the distance request page, browser will find the function corresponding to |
|
@hayata-suenaga bump |
|
ah I thought that separate files are fetched with module imports. Let's go with module import approach @pradeepmdk sorry for the late response 🙇 |
|
@hayata-suenaga np. update now to module import @ntdiary |
|
@pradeepmdk, There are two pending tasks need to be addressed, feel free to let me know if there are any problems. : ) |
pradeepmdk
force-pushed
the
fix/26589-map-route-is-not-genrated
branch
from
5534443 to
195b191
Compare
sorry @ntdiary i am not able to see this in review comments so that missed this. i am updated now.. |
Yeah, they have confirmed they will fix that case, so we can focus on our own scenario here. |
|
@ntdiary updated. |
|
@pradeepmdk, great! Could you please explain this? And we are very close to the end. 😄 |
yes i agree we don't need when i am testing i removed the token from indexdb manually deleted so callback its not triggering so that i added this one. @ntdiary one small doubt how you are adding this review comment because i am not able see this here.🤔 and |
pradeepmdk
force-pushed
the
fix/26589-map-route-is-not-genrated
branch
from
7282837 to
5b7f27e
Compare
@pradeepmdk, ah, my bad, I forgot to submit the review, and will start testing later. 😂 |
Reviewer Checklist
Screenshots/VideosWeb26998-web.mp4Mobile Web - Chrome26998-mobile-chrome.mp4Mobile Web - Safari26998-mobile-safari.MP4Desktop26998-desktop.mp4iOS26998-mobile-ios.MP4Android26998-mobile-android.mp4 |
@pradeepmdk, let's update the |
|
@ntdiary updated |
|
@hayata-suenaga, kindly bump. : ) |
|
@hayata-suenaga, bump; |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/hayata-suenaga in version: 1.3.75-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.75-12 🚀
|
|
🚀 Deployed to staging by https://github.com/hayata-suenaga in version: 1.3.76-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.76-6 🚀
|
Details
Fixed Issues
$ #26589
PROPOSAL: #26589
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Screen.Recording.2023-09-08.at.6.51.39.AM.mov
Mobile Web - Chrome
Record_2023-09-08-07-28-28.mp4
Mobile Web - Safari
WhatsApp.Video.2023-09-08.at.7.15.05.AM.mp4
Desktop
Untitled.mov
iOS
WhatsApp.Video.2023-09-08.at.9.03.26.AM.mp4
Android
Record_2023-09-08-07-52-26.mp4