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

Improve Build System #871

Open
hyperupcall opened this issue Feb 20, 2023 · 3 comments
Open

Improve Build System #871

hyperupcall opened this issue Feb 20, 2023 · 3 comments
Labels
p-medium-priority t-enhancement This issues tracks a potential improvement to the software
Milestone

Comments

@hyperupcall
Copy link
Contributor

hyperupcall commented Feb 20, 2023

Is your feature request related to a problem? Please describe.

Looking through the build system files, I noticed a few things:

  • The output bundle has a total size of 15.5 MiB
  • plotly.js takes up ~30% of the final bundle
  • There is a babel.config.json and a babel key in package.json, but babel-loader currently isn't being activated for any files (JavaScript is ran through ts-loader)
  • Under resolve.fallback of the Webpack config, polyfills for buffer, assert, stream, etc. are specified (with NodePolyfillPlugin)

These issues can potentially cause broken code (code that hasn't been transpiled by Babel) to be shipped to browsers, or cause slow loading times.

Describe the solution you'd like

To make improvements, I suggest the following:

  • Decrease the output bundle size by
    • Lazy-loading code, for each of the different routes
    • Tree-shaking plotly.js and other dependencies from node_modules/
  • Remove resolve.fallback and NodePolyfillPlugin
    • Webpack 5 disabled automatic polyfillying of NodeJS modules since it makes it impossible for developers to accidentally include a NodeJS dependency from the frontend (increasing the bundle size). This webpack config partially undoes that, and since I couldn't find other code depending on this configuration, it would be good to remove this

It might also be useful to clarify if Babel should be used or not. And if so, what browsers and versions of browsers should be supported (I checked the website and found a mention of "all major web browsers"). And once we decide that, then to enable Babel, if applicable.

Describe alternatives you've considered

Leaving the Webpack configuration as it is

Additional context

I'd totally be down making a PR implementing this if it is desired

@huss
Copy link
Member

huss commented Feb 22, 2023

Thanks to @hyperupcall for looking into this. Let me start by saying this is not an area that I have any real knowledge.

OED planned to look into optimizations after v1.0 so it is a fine time to consider this. I have been concerned about the initial load time of OED and would like to get that down so reducing the bundle size would definitely help. Here are my general thoughts and maybe others can chime in:

  • I'm okay with changing how the loading is done. I think it would be good to try to keep the main graphic pages to be quick (and maybe all load at the start but a small delay in changing graphics would be okay as a tradeoff). The other pages are less common so they can afford a slight delay.
  • I'm okay with trying the other ideas to see how they work. I believe Webpack was the initial system used and ts-loader was added more recently.

Just for the record, I have thought about reducing the amount of data sent. I did this in export by reducing the name size used to identify items. We can also consider a long-standing question of what data to transfer when it is requested (currently we transfer all graphic types). These will not impact initial loading but could impact the speed when selections are made. It is unclear they will make much of a difference so more performance monitoring is needed to know. Finally, the DB queries could be optimized but the impact needs to be assessed. None of this impacts the idea above and I think they should move forward independently.

@hyperupcall
Copy link
Contributor Author

I'm okay with changing how the loading is done. I think it would be good to try to keep the main graphic pages to be quick (and maybe all load at the start but a small delay in changing graphics would be okay as a tradeoff). The other pages are less common so they can afford a slight delay.

Sounds good 👍 . I also think a slightly delay in loading for the non-primary page is a good tradeoff as well.

I'm okay with trying the other ideas to see how they work. I believe Webpack was the initial system used and ts-loader was added more recently.

For sure, I'll also provide some numbers showing the improvements so we know how much the new ideas work.

Just for the record, I have thought about reducing the amount of data sent. I did this in export by reducing the name size used to identify items.

If I understand your comment correctly, I don't think this has much of an impact, since for production, the names are ran through a terser

None of this impacts the idea above and I think they should move forward independently.

👍

@huss
Copy link
Member

huss commented Feb 22, 2023

I'm okay with changing how the loading is done. I think it would be good to try to keep the main graphic pages to be quick (and maybe all load at the start but a small delay in changing graphics would be okay as a tradeoff). The other pages are less common so they can afford a slight delay.

Sounds good 👍 . I also think a slightly delay in loading for the non-primary page is a good tradeoff as well.

I say give it a go and see how it works.

I'm okay with trying the other ideas to see how they work. I believe Webpack was the initial system used and ts-loader was added more recently.

For sure, I'll also provide some numbers showing the improvements so we know how much the new ideas work.

Sounds great.

Just for the record, I have thought about reducing the amount of data sent. I did this in export by reducing the name size used to identify items.

If I understand your comment correctly, I don't think this has much of an impact, since for production, the names are ran through a terser

That makes sense as I tested in development. Good to know this and I might change the names back but they are only for export so not too important. That is really slow for raw data and nice that a developer can do it quicker for testing.

Thanks for taking this on and I look forward to seeing the result.

@huss huss added this to the 1.1 release milestone Mar 24, 2024
@huss huss added t-enhancement This issues tracks a potential improvement to the software p-medium-priority labels Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p-medium-priority t-enhancement This issues tracks a potential improvement to the software
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants