Proposal for switching desktop-client to vite #2084
Conversation
trafico-bot
bot
added
the
🚧 WIP
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
twk3
force-pushed
the
poc-desktop-client-vite
branch
3 times, most recently
from
0ca51ef
to
c0926be
Compare
|
This is exciting! Took a stab before at implementing Vite bit but encountered issues with the inter-ui fonts as well as resolution errors. |
twk3
force-pushed
the
poc-desktop-client-vite
branch
from
2aa3855
to
6dc8cf1
Compare
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
View detailed bundle breakdownAdded No assets were added Removed
Bigger No assets were bigger Smaller No assets were smaller Unchanged No assets were unchanged |
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller
Unchanged No assets were unchanged |
twk3
force-pushed
the
poc-desktop-client-vite
branch
from
6dc8cf1
to
e71dda9
Compare
|
👋 Could you list out the pros/cons of the switch to vite? Do we have any performance, bundle size, DX benefits here? What cons would the switch have? And don't get me wrong: I'm not against this move. I just want to get a better sense of the potential drawbacks of this approach. A while ago we used to have loads of manually defined webpack files. They were a nightmare to maintain.. and upgrading webpack versions was super labour-intensive. So I made the decision to switch to react-scripts. Whilst it was not ideal - it did give us back a lot of dev capacity to focus on the product instead of maintaining build scripts. No matter the future tool we choose: I would prefer that it is low-maintenance. So we get to spend our limited time working on the product, not the tooling around it. |
twk3
force-pushed
the
poc-desktop-client-vite
branch
from
9417142
to
6afa78a
Compare
|
Fixed the netlify build so we can see it actually working. There is probably some better way that would allow us to keep using process.env, but currently process.env is always an empty object in a prod build, rather than being turned static) @MatissJanis I updated the description with some goals/pros cons that I've thought of. Of note this was just me trying a vite migration to see how it went, but it did actually go better than I was expecting, and might be worth consideration. (I'm happy to attempt a similar migration with another provider if someone wants to suggest some). My main goal is exploring ways to unlock dependency updates. react-scripts appears dead, and was getting in the way of trying a typescript upgrade. My first approach was to eject, but I did read the PRs introducing CRA back in, and craco, and agree with the desire to reduce the maintenance burden. As a result my hope was to actually find a react-scripts replacement that was still using webpack. With swc in there, I don't see a reason to change if possible. But I wasn't able to find a popular one that was still being maintained. (things like snowpack are suggesting vite). I was actually planning to see what remix, (and still calling out to webpack from it), would look like. But a recent comment in #656 caused me to make an attempt at vite first. I have to say, if you don't look at the yarn.lock, it actually dropped in pretty well. The big config file is mostly me trying to match the old build output and old config file. (so that this just drops into actual-server perfectly for example without any changes needed). There are lots of downsides, when you do look at the lockfile, you can see the expanse of the behind the scenes changes this swap does. If we merged this, we would be spending the next month fixing edge cases we broke. And then ongoing, vite is still maturing, and releasing major versions somewhat frequently. We don't get those nice dependency updates along for the ride unless we also update vite. (this MR already needs a new vite update just to fix the fonts). That being said, I do think this shows that vite is a viable option. And we will need something here in the next year when we want to update the dependencies. Let me know if there are others you want me to POC. |
|
Small comment, since I recently switched to Vite. Running Vite as dev server, it'll use esbuild, which in turn uses the React SWC plugin. In this case, minify is kinda irrelevant. If you build and bundle the project, that's all rollup instead, which doesn't use that plugin. |
|
Is there a Vite plugin to allow us to continue using process.env? I am most curious about the difference in build times and dev server reload time between the current and Vite so it would be great if we can measure that. |
twk3
force-pushed
the
poc-desktop-client-vite
branch
from
6afa78a
to
8029e33
Compare
@glowtape in this MR swc is also used for transform even in build mode. This is triggered by passing a swc specific plugin the vite-react-swc plugin. No clue if there is a benefit yet. I was trying to keep the output similar to what we already have, which is using swc. |
@joel-jeremy I went ahead and embedded one that fixes it so now process.env is working here. Of note both here, and previously, this was NOT the node process.env. (I saw a lot of stackoverflow suggestions to just dump node process env into the defines). Both CRA and vite are careful to clear out process.env. CRA just reused it for it's own env handling as well, whereas vite uses import.meta.env. The inject plugin gets it back in there, but I was previously running into issues with the fact that vite was coming through during build and purposely replacing any instance of process.env with an empty object. https://github.com/vitejs/vite/blob/main/packages/vite/src/node/plugins/define.ts#L20 It's working now. Was all about timing things around when vite was messing with process.env so it didn't wipe out the shim. Build times and size were similar to what we had before. Slightly longer on the build times (9 seconds vs 8 seconds on my machine, due to the inject plugin) And slightly smaller on the output sizes. I haven't done any experiments with dev server times, which is where we SHOULD be getting some benefit. |
|
@MatissJanis I moved the renames over into a MR here that can be reviewed and merged if wanted: #2101 turns on the eslint rule to enforce it the (t|j)sx filename. |
twk3
force-pushed
the
poc-desktop-client-vite
branch
from
43444b8
to
71683f3
Compare
|
Fonts are worked around now, and this has been rebased to include the renames that went in seperately. Now it's easier to review the actual changes here. I haven't divined into why vite or its css is like this, but it's not resolving css urls relative to the node_module the css file is in. Using import directly of a scss file in the module works, but doing it via We have some options to work around, what I've done here is just set the font path variable to go to the proper location. But we could also switch to an |
twk3
force-pushed
the
poc-desktop-client-vite
branch
from
943898c
to
7ccdd6f
Compare
|
After looking around and not really finding an good alternative that didn't involve a third party monitoring service, I ported and published a github action that gives us a sizecompare for rollup. https://github.com/marketplace/actions/rollup-comparison (just the webpack one, but modified to map the stats file from rollup visualizer) I also removed the manual chunks. As the default is already patching the number that webpack had (with a bit smaller size), and maintaining a manual list seemed like a pain. I had only added them because rollup was complaining about the chunk sizes, so I've bumped up the warning threshold. Remaining items that I know of is to look into the victory warnings, and see if I can see a dev environment load improvement. |
|
Tracked down the victory warnings. Looks like it's an incompatibility between babel output and rollup's PURE annotation implementation. I logged an issue with rollup here: rollup/rollup#5324 The impact (other than the warnings), is that tree shaking isn't working to remove extra code in the victory components. While nice to fix, the fact that our bundle sizes are already smaller with rollup probably makes this a non-issue for us. I'm going to take a look at filtering out these warnings for now to reduce some noise. |
packages/desktop-client/src/components/transactions/TransactionsTable.jsx
Outdated
Show resolved
Hide resolved
twk3
changed the title
trafico-bot
bot
added
🔍 Ready for Review
|
Merged the electron PR. Apologies for creating merge conflicts here @twk3 |
Just set the limit higher
- This autogenerates self-signed certs in dev mode when HTTPS env is set - Made to match the CRA behaviour
twk3
force-pushed
the
poc-desktop-client-vite
branch
from
d411837
to
e5e57a2
Compare
trafico-bot
bot
added
🔍 Ready for Review
- Updated to a rollup version that includes the fix
|
Added one more commit. Rollup released a new version that fixed the victory warnings rollup/rollup#5324 so I was able to drop the warning suppression we had for that from the vite config |
trafico-bot
bot
added
✅ Approved
trafico-bot
bot
added
✨ Merged
* Proof of concept for switching desktop-client to vite * Fix other packages ts tests issues * Update jsx tests to use vitest instead of jest * Inject our global shims properly * Add comment regarding new plugin * Cleanup unnessary change after rebase * Fix inter fonts pathing * Remove manual chunks sizes for now Just set the limit higher * Bring back size compare * Suppress victory warnings * Remove craco config now that it's not used * Add vite basic ssl plugin - This autogenerates self-signed certs in dev mode when HTTPS env is set - Made to match the CRA behaviour * Add release note * Remove warning suppression for victory - Updated to a rollup version that includes the fix
Proposal for vite switch for #656
Why we need to switch from CRA/craco
They appear to be deprecated, and not getting their dependencies updated. That's a problem because it makes it a lot more difficult to keep actual's dependencies up-to-date. It's currently difficult to update to the latest typescript as an example.
What we want
Pros of vite
Cons of vite
-Will need to build our own custom transform for the github action size compare (as it only works for webpack). This is outputing the stats, but we don't have a consumer.
Speed and size
PR details
Major changes