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.

Sign up for GitHub

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

Jump to bottom

#8535: Additional improvement on the main bundle #8531

Merged
merged 9 commits into from
Sep 2, 2022
Merged

#8535: Additional improvement on the main bundle #8531

merged 9 commits into from
Sep 2, 2022

Conversation

allyoucanmap
Copy link
Contributor

@allyoucanmap allyoucanmap commented Aug 31, 2022 •
edited
Loading

Description

This PR includes improvement related to:

  • Importing components libraries only when they are mounted in the UI using lazy + suspense. Here the libraries:
    • react-codemirror
    • react-draft-wysiwyg
    • pdf.js
    • react-quill
    • react-data-grid
    • pdf.js
    • vis-timeline-graph2d.min
  • Completely removed css-tree in favor of static json configuration
  • Importing only the woff2 font for icons in the css file
  • made the browser valid check script async in the html page
  • remove leaflet cdn script and css from html pages in favor of dynamic loading

This PR introduces also @testing-library/react to simplify test of lazy react components

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Other... Please describe: Improvement on dependencies loading

Issue

What is the current behavior?

#8535

What is the new behavior?

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • Yes, and I documented them in migration notes

Other useful information

offtherailz
offtherailz requested changes Aug 31, 2022
View reviewed changes
Copy link
Member

@offtherailz offtherailz left a comment •
edited by allyoucanmap
Loading

Choose a reason for hiding this comment

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

I can not fully review.

  • I can suggest to add the "Migration guidelines" for html files.

Apart from this PR we may do a raw test to see how the score improves if we setup caching for every js/css file. If it makes the difference, we should make the main

  • bundles
  • css
  • extensions main files should be excluded for the moment(index.js has not hash)
    to have the hash in the name, so caching can be applied easely and updates will load new files.

@allyoucanmap allyoucanmap self-assigned this Sep 1, 2022
@allyoucanmap allyoucanmap changed the title Additional improvement on the main bundle #6735: Additional improvement on the main bundle Sep 1, 2022
@allyoucanmap allyoucanmap marked this pull request as ready for review September 1, 2022 09:27
@tdipisa tdipisa requested a review from MV88 September 1, 2022 09:30
@tdipisa tdipisa requested a review from offtherailz September 1, 2022 10:48
offtherailz
offtherailz requested changes Sep 1, 2022
View reviewed changes
Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

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

LGTM for my inspection. Just fix the suggested text.

docs/developer-guide/mapstore-migration-guide.md Outdated Show resolved Hide resolved
@allyoucanmap @offtherailz
Update docs/developer-guide/mapstore-migration-guide.md
d10338a 
Co-authored-by: Lorenzo Natali <lorenzo.natali@geosolutionsgroup.com>
@allyoucanmap allyoucanmap mentioned this pull request Sep 1, 2022
Closed
6 tasks
@allyoucanmap allyoucanmap changed the title #6735: Additional improvement on the main bundle #8535: Additional improvement on the main bundle Sep 1, 2022
@tdipisa tdipisa added C169-Rennes-Métropole-2021-GeOrchestra3 BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch labels Sep 1, 2022
@tdipisa tdipisa requested a review from offtherailz September 1, 2022 12:49
MV88
MV88 requested changes Sep 1, 2022
View reviewed changes
Copy link
Contributor

@MV88 MV88 left a comment

Choose a reason for hiding this comment

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

Just a few comments here and there, mostly related to the documentation

anyway the app seems faster

web/client/components/background/BackgroundSelector.jsx Show resolved Hide resolved
web/client/components/misc/withSuspense.jsx Outdated Show resolved Hide resolved
web/client/components/misc/withSuspense.jsx Show resolved Hide resolved
web/client/components/styleeditor/Editor.jsx Outdated Show resolved Hide resolved
web/client/libs/codemirror/react-codemirror2.js Show resolved Hide resolved
web/client/reducers/mapInfo.js Outdated Show resolved Hide resolved
allyoucanmap and others added 3 commits September 1, 2022 18:11
@allyoucanmap @MV88
Update web/client/components/misc/withSuspense.jsx
d9b83e0 
Co-authored-by: Matteo V. <matteo.velludini@geosolutionsgroup.com>
@allyoucanmap @MV88
Update web/client/components/styleeditor/Editor.jsx
56c84f5 
Co-authored-by: Matteo V. <matteo.velludini@geosolutionsgroup.com>
@allyoucanmap
fixes from review
dbbb281
@allyoucanmap allyoucanmap requested a review from MV88 September 1, 2022 16:30
offtherailz
offtherailz approved these changes Sep 2, 2022
View reviewed changes
Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

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

Thank you for the changes on doc. 👍

@allyoucanmap allyoucanmap merged commit a7c626b into geosolutions-it:master Sep 2, 2022
@allyoucanmap
Copy link
Contributor Author

allyoucanmap commented Sep 2, 2022 •
edited
Loading

@tdipisa @ElenaGallo this PR can be tested on DEV, we should compare the initial loading with QA and Release instances. As soon as everything is tested we can backport to stable branch 2022.02.xx

@MV88
Copy link
Contributor

MV88 commented Sep 2, 2022

@allyoucanmap can you start preparing the backport?

@MV88 MV88 mentioned this pull request Sep 5, 2022
Closed
1 task
alexander-fedorenko pushed a commit to alexander-fedorenko/MapStore2 that referenced this pull request Sep 7, 2022
@allyoucanmap @alexander-fedorenko
geosolutions-it#8535: Additional improvement on the main bundle (geos…
8235f0c 
…olutions-it#8531)

(cherry picked from commit a7c626b)
@alexander-fedorenko alexander-fedorenko mentioned this pull request Sep 7, 2022
Merged
@ElenaGallo ElenaGallo removed the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Sep 7, 2022
@allyoucanmap allyoucanmap mentioned this pull request Sep 14, 2022
Closed
1 task
@offtherailz offtherailz added this to the v2022.02.00 milestone Sep 28, 2022
@allyoucanmap allyoucanmap mentioned this pull request Mar 3, 2023
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@offtherailz offtherailz offtherailz approved these changes

@MV88 MV88 MV88 approved these changes

Assignees

@allyoucanmap allyoucanmap

Labels
C169-Rennes-Métropole-2021-GeOrchestra3 enhancement Priority: High
Projects
None yet
Milestone
2022.02.00
Development

Successfully merging this pull request may close these issues.
5 participants
@allyoucanmap @MV88 @offtherailz @tdipisa @ElenaGallo