Fix new CSS assets not being used after a rebuild #1100
Conversation
Gruntfile.js
Outdated
| if (['.css', '.sass', '.scss'].includes(extension)) { | ||
| // Esbuild outputs `main.css` and `main.css.map` along `main.js`, | ||
| // but `index.html` wants to load them from `dist/assets/css`. | ||
| await fs.promises.copyFile(`${distJS}/main.css`, `${distCSS}/main.css`) | ||
| if (buildMain.options.sourcemap) { | ||
| await fs.promises.copyFile(`${distJS}/main.css.map`, `${distCSS}/main.css.map`) | ||
| } |
There was a problem hiding this comment.
This seems very granular of a way to do it, very specific to esbuild as well, which will make it more difficult to close #1085, I think. Have you considered doing it using the existing grunt-contrib-copy stuff that we have, or in a more generic way that is less specific to esbuild integration?
There was a problem hiding this comment.
Indeed, and it also introduces a bit of code duplication since the same copying operation occurs on the first build.
So I've considered specifying this post-build operation as a custom option, then thought it was better to keep things simple for now; but I should be able to add it if you don't mind the extra option.
I it is also possible via grunt copy, but I think it would still be as much specific to esbuild as plain code, and likely a bit slower. I'm also slightly reluctant to increase our dependency to Grunt since we might want to switch to Snowpack later.
There was a problem hiding this comment.
Would the post-build option would help with #1085 and DRY the code more?
If so, then I think it's worth doing.
There was a problem hiding this comment.
I think so since this copying operation might not be needed with other build tools, or they might require doing something else.
There was a problem hiding this comment.
Per the convo here: https://github.com/okTurtles/group-income-simple/pull/1100/files#r710204644
Implement the copying part using a post-build option to DRY things and help with #1085
There was a problem hiding this comment.
Approved! Great catch @snowteamer! (My bad for not catching it originally!)
Closes #1099
Fixes the relevant issue by copying new
main.cssandmain.css.mapover to the correct location.