Skip to content
This repository has been archived by the owner on Jun 10, 2024. It is now read-only.

feat: lazy loading to modals #358

Open
wants to merge 1 commit into
base: epic-v2-save-load
Choose a base branch
from

Conversation

rostyslavvnahornyi
Copy link
Collaborator

Hi @kfarr,
The bundle originally is 1.75mb. After implementing lazy loading and code splitting, the bundle size reduced to 1.67MB.

So, is it necessary to add more routes? If so, which routes should be added? Adding routes doesn't reduce the bundle size, instead, it remains the same. Better approach would be to add webpack split chunks, as it provides a similar impact to routing.

There is a root file – inspector, and introducing new routes to the root file adds more problems. Moreover, adding new routes requires adjusting the logic with urls to address and fixing any related bugs, which can be time-consuming.

Maybe is better solution to leverage webpack split chunks instead of introducing additional routes?

@kfarr
Copy link
Contributor

kfarr commented Dec 4, 2023

@rostyslavvnahornyi thanks for this feedback. Here are some ideas on how to proceed. Please let me know your thoughts. If it makes sense, then I will need to create some separate tickets for these various items:


My understanding of routes would be that the "right" answer is to have proper routes that match with the user interface and roughly map to modals and move scene UUID to the path instead of hash. So given the structure of https://URL.com/path/#route we could imagine the following routes in the 3DStreet application:

  • #open
  • #share
  • #help
  • #account or #profile or #login for auth related activities
  • #edit
  • #view (not an actual React route, instead if present upon pageload, #view would actually prevent editor from loading and just fetch a scene in viewer mode only)

This structure implies a server-side change to use the path for scene ID instead of the hash:

  • instead of 3dstreet.app/* returning index.html configured as a "single page application" , the current ID of a street when saved or loaded is shown in the URL path, such as https://3dstreet.app/scenes/UUID/#route leaving #route available for this new use case.

Other considerations:

  • We still need to support legacy use cases where the application is loaded from a third-party server such as Streetmix and provides a Streetmix URL or 3dstreet JSON URL in the hash such as this structure: https://3dstreet.app/#https://streetmix.net/kfarr/58/... I propose that in such cases we handle this with 3dstreet javascript to detect if present, then to load the scene, then to clear the hash and enter editor mode #editor.
  • We'll have to double check that there is no remaining logic in the app to use hash for fetching scene ID. Instead this should use the global STREET object helper method to get current ID from scene metadata.

@rostyslavvnahornyi
Copy link
Collaborator Author

Hi @kfarr.
Thanks for your ideas! Waiting for you to create separate tickets.

@kfarr
Copy link
Contributor

kfarr commented Dec 5, 2023

@rostyslavvnahornyi I was looking for some feedback from you -- does this plan work? Or are there things I'm not considering? What about the routes -- are those the right routes or are there some missing? What about the fact that toolbar.js includes save / load features -- does that mean that we should extract those elsewhere so they aren't loaded until needed so the #edit route is smaller?

@rostyslavvnahornyi
Copy link
Collaborator Author

@kfarr, the plan is good. I think your point about the toolbar is valid, and we need to move it under the viewer route, but It might be difficult.

The other routes are correct; we've covered almost all aspects of the application with these routes, and nothing has been overlooked.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants