Conversation
in ci, electron build attempts to rebuild fsevents@1.2.9, which is only used as a dev dep. should potentially be okay to omit
- use window.electron to access electron imports - use require for importing external modules (mostly node built-ins)
This reverts commit 163a0ab.
ErikSin
reviewed
Jun 9, 2023
| } | ||
| } | ||
|
|
||
| await runCommand(extractArgs()) |
Contributor
There was a problem hiding this comment.
top level await...the future is now
ErikSin
reviewed
Jun 9, 2023
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' }, |
Contributor
There was a problem hiding this comment.
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?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Goals
Notable changes (so far)
esbuild.mjs. all configuration and any related build steps are performed there.build: runs the bundler process in its entirety and output to thestaticdirectory. accepts a--modearg of eitherdev(default) orprod. as of nowprodwill 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 thestatic/directory. accepts a--modearg of eitherdev(default) orprod. as of nowprodwill include minification, but should do other things when we're ready for that.clean: cleans built assets that are outputted in thestatic/directory./esbuild.mjs build: compiles the renderer code and outputs it to thestatic/directory./esbuild.mjs watch: compiles the renderer code incrementally and starts the dev server that serves thestatic/directory (defaults tohttp:/localhost:8000).main.htmlfile. this allows for live-reloading (note, not hot module reloading or react fast refresh). see thepreload/main.jspreload script to see how that's done (it's just an eventsource that callslocation.reloadwhen a change is detected).src/rendererdirectory. 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.src/preloaddirectory. 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).staticdirectory. this way we don't deal with configuring loaders for certain assets right now, which was causing some asset duplication.react-intluses 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 todefineMessagesto manually specify something like{ id: ..., defaultMessage: ...}for each message string. Based on the lack of a diff in the generatedmessages/, i think my change has been completed successfully. Just need to keep this in mind moving forward.main.html)windowin themain.jspreload 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:
externalhave to be imported viarequirewhen 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 likeconst path = require('path'). this is not really a blocking issue, but just a notable change that may be slightly inconvenient.Remaining work
pathmodule such as building a file path viapath.join, it should be abstracted to a file located somewhere likesrc/renderer/node/path.jsas a helper function, which is then used by the component. my reasoning for this:builder.config.jsconfig 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.