feat: Add lazy import for different routes #656

Sudhanva-Nadiger · 2023-07-04T15:05:26Z

imported routes from lazy imports
added suspense wrapper around all pages

What

since there are many routes in the app, we can delay js bundle file downloads by adding lazy imports

Screenshot

Build Results

Without lazy import
With lazy import

- imported routes from lazy imports - added suspense wrapper around all pages

src/pages/flaggedImages/index.jsx

+                <td>
+                  <a
+                    target="_blank"
+                    href={off.getProductEditUrl(code)}


- when file is still loading data wont be there. so added the check if its is not null in opportunities.tsx

Sudhanva-Nadiger · 2023-07-04T16:53:44Z

@alexfauquette I tested this in my local machine with the hosted website! it worked :)!! But i was thinking that we should probably add tests to make sure things are not breaking while improving the code base.

alexfauquette · 2023-07-05T07:09:11Z

Hi, Effectively having tests to ensure all games are still working could be nice. But for now, there is nothing so it's to build from scratch

About reducing the bundle size, you might be interested by the webpack.config.js files. It comes from the first version of this project (more than 5 years ago) and I never touched it. What I understand is that it tends to merge all the files in a single one.

Sudhanva-Nadiger · 2023-07-05T07:30:49Z

When I build it made chunks of js files!

and webpack does make chunks of js files when using react lazy imports (ref: https://uploadcare.com/blog/lazy-loading-in-webpack/)

Sudhanva-Nadiger · 2023-07-05T13:28:11Z

So should I close this pr ?

alexfauquette · 2023-07-06T12:16:29Z

So should I close this pr ?

I tested it too and it's fine so we can merge it if you want, but I struggle to understand the benefit.

The first one is personal (pure feeling): I never felt the interface was long to download. It's more call to the different API that take times
The second one is technical: Your PR split pages (about 10 different pages) but the result if 4 JS files. So their might be a problem somewhere

It seems that if you don't use the webpack config (run yarn build:react instead of yarn build) you get much more JS files. I did not try to if it was working or not

@VaiTon would you have an opinion on this topic?

Sudhanva-Nadiger · 2023-07-06T12:41:49Z

There is only 4 chunks ! rest all are maps and license, even with normal one it happens

Sudhanva-Nadiger · 2023-07-06T12:45:34Z

The first one is personal (pure feeling): I never felt the interface was long to download. It's more call to the different API that take times

I am not sure either,
but what I feel is: when we have room for the decrement of the initial load/improvement then why not?

VaiTon · 2023-07-07T16:11:23Z

The second one is technical: Your PR split pages (about 10 different pages) but the result if 4 JS files. So their might be a problem somewhere.

I think there is a problem about having react-scripts and webpack. The build commands run them sequentially, but they do the same thing. In fact the react-scripts build is splitted more than the webpack build as far as I understand. I tested a clean build with only yarn build:react and it seems to work.

I recommend removing the other command.

VaiTon · 2023-07-07T16:15:27Z

In the long run I'd recommend switching to Vite too

VaiTon · 2023-07-07T16:20:47Z

The build commands run them sequentially, but they do the same thing.

Ok so I read again the webpack file, and it appears that it takes the chunks and output a bundle.min.js, which is then unused.

From the README

Use a simple webpack config to bundle in a single file (build/bundle.min.js) and facilitate integration in OFF main site

VaiTon · 2023-07-07T16:28:26Z

I've created #657 for the webpack issue

VaiTon · 2023-07-07T16:40:46Z

Also, for me this branch run with yarn build:react produces

Compiled successfully.

File sizes after gzip:

  281.01 kB  build/static/js/main.e66acd1a.js
  83.42 kB   build/static/js/996.15642c50.chunk.js
  26.14 kB   build/static/js/416.d8f9f62d.chunk.js
  13.92 kB   build/static/js/865.6d0cd710.chunk.js
  12.97 kB   build/static/js/635.1a264e73.chunk.js
  12.01 kB   build/static/js/247.992838d8.chunk.js
  11.85 kB   build/static/js/81.8b4229f5.chunk.js
  9.2 kB     build/static/js/636.09ffedf7.chunk.js
  8.68 kB    build/static/js/915.7c9d507e.chunk.js
  8.48 kB    build/static/js/250.a74eb122.chunk.js
  8.24 kB    build/static/js/994.f77743c9.chunk.js
  7.11 kB    build/static/js/182.75d07425.chunk.js
  6.98 kB    build/static/js/713.85a70955.chunk.js
  6.69 kB    build/static/js/848.40591dcf.chunk.js
  6.1 kB     build/static/js/609.4b4f8846.chunk.js
  5.86 kB    build/static/js/965.6653e7a5.chunk.js
  5.31 kB    build/static/js/861.0f881878.chunk.js
  4.97 kB    build/static/js/752.c72f6729.chunk.js
  4.95 kB    build/static/js/331.fea3d7e7.chunk.js
  4.94 kB    build/static/js/230.58cd0b2d.chunk.js
  4.78 kB    build/static/js/59.cc5a4fcf.chunk.js
  4.41 kB    build/static/js/97.1899b77a.chunk.js
  4.04 kB    build/static/js/557.71266763.chunk.js
  3.72 kB    build/static/js/424.47c62fd0.chunk.js
  3.71 kB    build/static/js/44.d44f2428.chunk.js
  3.71 kB    build/static/js/369.99374898.chunk.js
  3.6 kB     build/static/js/964.8fe29e93.chunk.js
  3.55 kB    build/static/js/849.0b6340b0.chunk.js
  3.41 kB    build/static/js/595.a8885faf.chunk.js
  3.38 kB    build/static/js/280.51f7ffc8.chunk.js
  2.75 kB    build/static/js/854.8c0f7acb.chunk.js
  2.62 kB    build/static/js/358.265755dd.chunk.js
  2.57 kB    build/static/js/900.c60f7e12.chunk.js
  2.54 kB    build/static/js/378.f2122baf.chunk.js
  2.48 kB    build/static/js/425.5b353d8c.chunk.js
  2.47 kB    build/static/js/588.871fe44d.chunk.js
  1.55 kB    build/static/js/5.5fa67bab.chunk.js
  942 B      build/static/js/563.80b37c33.chunk.js
  507 B      build/static/js/650.921ce724.chunk.js
  323 B      build/static/js/493.70854ce4.chunk.js

alexfauquette · 2023-07-07T18:59:08Z

Yes, I think the "facilitate integration in OFF main site" was an initial idea that never got implemented

@teolemon could you confirm the main website does not show directly hunger games?

teolemon · 2023-07-07T21:27:58Z

The only thing left: https://world.openfoodfacts.org/hunger-game

Sudhanva-Nadiger · 2023-07-08T02:08:08Z

In the long run I'd recommend switching to Vite too

Yess the tooling is next level, and also hmr is great ! Makes development process great !

If we are moving from webpack to vite there is a plugin for migration: https://github.com/originjs/webpack-to-vite

alexfauquette · 2023-07-08T08:18:30Z

Yess the tooling is next level, and also hmr is great ! Makes development process great !

Ok, I don't know that much about this framework, I assume it's similar to NextJS

Should we close this PR and open a new one with vite?

VaiTon · 2023-07-08T14:16:15Z

@alexfauquette we could still use lazy loading. So we could merge this and then I'll open a PR to use Vite.

feat: Add lazy import for diffrent routes

2525c7a

- imported routes from lazy imports - added suspense wrapper around all pages

Sudhanva-Nadiger requested a review from a team as a code owner July 4, 2023 15:05

github-actions bot added Nutrition game 🖼️ Logo game 🌱 Eco-Score game questions game insights labels Jul 4, 2023

github-actions bot assigned Sudhanva-Nadiger Jul 4, 2023

github-advanced-security bot found potential problems Jul 4, 2023

View reviewed changes

src/pages/flaggedImages/index.jsx

<td>

<a

target="_blank"

href={off.getProductEditUrl(code)}

Check warning

Code scanning / CodeQL

Client-side URL redirect

Untrusted URL redirection depends on a [user-provided value](1).

Sudhanva-Nadiger closed this Jul 4, 2023

fix: add fall back to app.js

b31a33f

- when file is still loading data wont be there. so added the check if its is not null in opportunities.tsx

Sudhanva-Nadiger reopened this Jul 4, 2023

VaiTon mentioned this pull request Jul 8, 2023

Use Vite instead of CRA #663

Merged

alexfauquette approved these changes Jul 9, 2023

View reviewed changes

VaiTon merged commit b9d4f93 into openfoodfacts:master Jul 9, 2023

github-actions bot mentioned this pull request Sep 26, 2024

chore(master): release 1.0.0 #1038

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add lazy import for different routes #656

feat: Add lazy import for different routes #656

Sudhanva-Nadiger commented Jul 4, 2023

Sudhanva-Nadiger commented Jul 4, 2023

alexfauquette commented Jul 5, 2023

Sudhanva-Nadiger commented Jul 5, 2023

Sudhanva-Nadiger commented Jul 5, 2023

alexfauquette commented Jul 6, 2023

Sudhanva-Nadiger commented Jul 6, 2023

Sudhanva-Nadiger commented Jul 6, 2023 •

edited

Loading

VaiTon commented Jul 7, 2023

VaiTon commented Jul 7, 2023

VaiTon commented Jul 7, 2023

VaiTon commented Jul 7, 2023

VaiTon commented Jul 7, 2023

alexfauquette commented Jul 7, 2023

teolemon commented Jul 7, 2023

Sudhanva-Nadiger commented Jul 8, 2023

alexfauquette commented Jul 8, 2023

VaiTon commented Jul 8, 2023

feat: Add lazy import for different routes #656

feat: Add lazy import for different routes #656

Conversation

Sudhanva-Nadiger commented Jul 4, 2023

What

Screenshot

Build Results

Sudhanva-Nadiger commented Jul 4, 2023

alexfauquette commented Jul 5, 2023

Sudhanva-Nadiger commented Jul 5, 2023

Sudhanva-Nadiger commented Jul 5, 2023

alexfauquette commented Jul 6, 2023

Sudhanva-Nadiger commented Jul 6, 2023

Sudhanva-Nadiger commented Jul 6, 2023 • edited Loading

VaiTon commented Jul 7, 2023

VaiTon commented Jul 7, 2023

VaiTon commented Jul 7, 2023

VaiTon commented Jul 7, 2023

VaiTon commented Jul 7, 2023

alexfauquette commented Jul 7, 2023

teolemon commented Jul 7, 2023

Sudhanva-Nadiger commented Jul 8, 2023

alexfauquette commented Jul 8, 2023

VaiTon commented Jul 8, 2023

Sudhanva-Nadiger commented Jul 6, 2023 •

edited

Loading