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

WIP: use esbuild for building main renderer code #722

Draft
wants to merge 46 commits into
base: master
Choose a base branch
from
Draft

Conversation

achou11
Copy link
Member

@achou11 achou11 commented Feb 28, 2023

Goals

  • Improve dev experience for writing renderer code. Initially thought that this would also be a good opportunity to investigate bundling the main process code, but decided against it because 1) we don't currently do that so it would be increasing the complexity of this work and 2) previous experiments suggest that Rollup may be a better fit for bundling node code.
    • things within scope include but are not limited to: supporting TypeScript, auto reload on source changes (renderer only), faster build times.
  • Reduce tooling complexity and dependency. Things like Babel, Webpack, etc tend to add a lot of maintenance overhead and subject us to keeping up with ecosystems to fix things or maintain compatibility. This work is an attempt to understand if having more control in these processes could be more maintainable for us moving forward, without relying as much on the ecosystem.

Notable changes (so far)

  • using ESBuild instead of webpack for building renderer code
    • as of now, the only relevant script for bundling is esbuild.mjs. all configuration and any related build steps are performed there.
    • as of now, the following commands are available:
      • build: runs the bundler process in its entirety and output to the static directory. accepts a --mode arg of either dev (default) or prod. as of now prod will include minification, but should do other things when we're ready for that.
      • watch: runs the bundler process in watch mode, which will create a dev server that serves the static/ directory. accepts a --mode arg of either dev (default) or prod. as of now prod will include minification, but should do other things when we're ready for that.
      • clean: cleans built assets that are outputted in the static/ directory
      • example commands:
        • ./esbuild.mjs build: compiles the renderer code and outputs it to the static/ directory
        • ./esbuild.mjs watch: compiles the renderer code incrementally and starts the dev server that serves the static/ directory (defaults to http:/localhost:8000).
  • in development, the electron main window now loads the dev server url instead of the static main.html file. this allows for live-reloading (note, not hot module reloading or react fast refresh). see the preload/main.js preload script to see how that's done (it's just an eventsource that calls location.reload when a change is detected).
    • note that live-reloading is only enabled for changes that occur in the src/renderer directory. if you make changes to source code in the main process, background processes, or preload scripts, you'll still have to manually stop restart the server to see those changes.
  • reorganization of some of the source files. for example,
    • preload scripts now live in the src/preload directory. we can choose to bundle/process these but for now, we don't because they work in a different environment (access to both browser and Node apis because Node integration is turned on).
    • removed any direct asset imports from source code (e.g. importing an image or font file into js) and instead updated the source code to refer to the static directory. this way we don't deal with configuring loaders for certain assets right now, which was causing some asset duplication.
      • this may change in the future, but there aren't any notable benefits to being able to import asset paths directly right now.
    • removed some static assets that didn't seem to be used by the app. there's a small chance that these are used by some dependencies (e.g. i know https://github.com/digidem/mapeo-settings does rely on some assets existing in the project), so may need to evaluate those changes carefully to make sure i didn't break something subtle.
  • removal of babel, including the transform that react-intl uses for auto id-ing messages based on file path. as a result, the flow for defining messages is identical to how we do it in Mapeo Mobile. i had to update every call to defineMessages to manually specify something like { id: ..., defaultMessage: ...} for each message string. Based on the lack of a diff in the generated messages/, i think my change has been completed successfully. Just need to keep this in mind moving forward.
  • changing the main app to load as an ES module instead of requiring the app bundle (see diff for main.html)
  • no direct import of electron apis in renderer code. instead, we attach apis to the window in the main.js preload script, which like a partial recommendation for when node integration is enabled (Error while importing electron in react | import { ipcRenderer } from 'electron' electron/electron#9920 (comment))

Known issues

Let it be known that I'm not a bundling wizard. It's very possible that the build configuration I've implemented so far needs major reworking based on how browsers and Electron code work. Some other notable issues include:

  • the pdf report worker doesn't seem to work. if you try to preview the pdf in the observations tab, it just throws and returns the error state. probably something about the way the worker is being bundled that needs to be adjusted, but it's been difficult to debug.
  • any modules that are specified as external have to be imported via require when used in the main renderer code. for example, most of the node apis are listed as external and can only be used by doing something like const path = require('path'). this is not really a blocking issue, but just a notable change that may be slightly inconvenient.

Remaining work

  • think it would be useful to consolidate any renderer code that uses node apis in a specific directory in the renderer. for example, if a component does some stuff with the path module such as building a file path via path.join, it should be abstracted to a file located somewhere like src/renderer/node/path.js as a helper function, which is then used by the component. my reasoning for this:
    • it makes it easier to understand which components rely on node apis. right now, it's a matter of knowing what deps we have that are node-specific and then grepping the codebase to see which renderer components import the various deps. with this re-organization, it will be much easier to just check for files that import from this node-specific directory. this approach probably won't work for all node-specific deps that we use, but if we can cover most of them, i think that's significantly helpful.
    • having the separation will make it easier to eventually move away from the enabled node integration for the renderer process. it's highly recommended against and there are pretty good references for how to go about it (see Error while importing electron in react | import { ipcRenderer } from 'electron' electron/electron#9920 (comment)), but they require changes in the renderer and main process code.
  • remove all of the webpack remnants. i haven't done this yet because i wanted a reference to the previous config to make sure the new process covers what we previously had.
  • tried my best to update the builder.config.js config to make sure that changes in output are accounted for in terms of what needs to be included/excluded when building the electron app. i'm sure i've missed some stuff there since i haven't specifically tested or focused on it yet.

ErikSin and others added 30 commits January 30, 2023 02:23
in ci, electron build attempts to rebuild [email protected], which is only
used as a dev dep. should potentially be okay to omit
Base automatically changed from es/chore-electron-upgrade to master March 8, 2023 20:25
}
}

await runCommand(extractArgs())
Copy link
Contributor

Choose a reason for hiding this comment

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

top level await...the future is now

Comment on lines +155 to +157
{ in: 'src/renderer/app.js', out: 'app' },
{ in: 'src/renderer/components/MapFilter/index.js', out: 'map-filter' },
{ in: 'src/renderer/components/MapEditor/index.js', out: 'map-editor' },
Copy link
Contributor

@ErikSin ErikSin Jun 9, 2023

Choose a reason for hiding this comment

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

app.js loads home.js, which loads map-filter, and map-editor. Does esbuild just know that, and keeps map-filter and map-editor out of the app bundle? Or do we need to indicate that in the home.js file like we do with webpack?

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

Successfully merging this pull request may close these issues.

2 participants