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: restore src-gen frontend production behavior #12950

Merged

Conversation

sdirix
Copy link
Member

@sdirix sdirix commented Sep 27, 2023

Restores the usage of 'require' in production mode when loading included modules.

At the moment, both in development as well as in production mode, the frontend will be generated using the 'import' function. Webpack will use this opportunity for code splitting, leading to the generation of many small bundles. As they are sequentially loaded, this has a negative effect on the startup speed of the frontend.

By using 'require' instead, webpack will produce a single bundle, which is faster to transmit without additional roundtrips. This behavior was used for production mode in all previous Theia versions and is now restored.

Contributed on behalf of STMicroelectronics

What it does

Use require for importing frontend modules in production mode, use import in the remaining modes (mainly development).

How to test

Trigger src-gen for the included example applications. Trigger with --mode development as well as --mode production.

Review checklist

Reminder for reviewers

Context

The require generation was removed with #12590

@sdirix sdirix force-pushed the restore-frontend-src-gen-production branch from c80e87a to 8e391de Compare September 27, 2023 14:46
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Does it make sense to replace all import statements with require? I.e. even in development build? Is there a benefit of using import at all in this context?

Note that we should definitely replace the import call at line 100.

@sdirix
Copy link
Member Author

sdirix commented Sep 27, 2023

Note that we should definitely replace the import call at line 100.

Ah I missed that! Good catch! Will update the PR.

Does it make sense to replace all import statements with require? I.e. even in development build? Is there a benefit of using import at all in this context?

Good questions, I assumed that the webpack build can be a bit faster, including in watch mode, if all modules are bundled separately. However I did not check whether that's really the case.

@sdirix sdirix force-pushed the restore-frontend-src-gen-production branch 2 times, most recently from b4b4fce to dedc6aa Compare September 27, 2023 15:01
@sdirix
Copy link
Member Author

sdirix commented Sep 27, 2023

I did a quick test with require vs import in the development build and could not find an improvement there in build times nor watch times. However I don't know whether we'll break somebody's use case if we now always generate require. If it's fine for you I would like to suggest to only restore the previous behavior for now, so that the feature is unchanged for the next release and discuss this further in the dev meetings or in the forums.

@sdirix sdirix force-pushed the restore-frontend-src-gen-production branch 3 times, most recently from 4a7316b to ed2025d Compare September 27, 2023 15:15
@sdirix sdirix requested a review from msujew September 27, 2023 15:25
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

Please rebase so I can merge :)

@msujew msujew added the theia-cli issues related to the theia-cli label Sep 27, 2023
Restores the usage of 'require' in production mode when loading
included modules.

At the moment, both in development as well as in production mode, the
frontend will be generated using the 'import' function. Webpack will
use this opportunity for code splitting, leading to the generation of
many small bundles. As they are sequentially loaded, this has a
negative effect on the startup speed of the frontend.

By using 'require' instead, weppack will produce a single bundle, which
is faster to transmit without additional roundtrips. This behavior was
used for production mode in all previous Theia versions and is now
restored.

Contributed on behalf of STMicroelectronics
@sdirix sdirix force-pushed the restore-frontend-src-gen-production branch from ed2025d to e80e819 Compare September 27, 2023 20:57
@msujew msujew merged commit 001a886 into eclipse-theia:master Sep 28, 2023
10 of 13 checks passed
@vince-fugnitto vince-fugnitto added this to the 1.42.0 milestone Sep 28, 2023
@sdirix sdirix deleted the restore-frontend-src-gen-production branch September 29, 2023 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theia-cli issues related to the theia-cli
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants