Enable Webpack ESM output and move assets to `/assets/` without subdirectories #25940

silverwind · 2023-07-17T23:00:53Z

Enable Webpack to output ESM aka. modules. This is still flagged experimental, but in my testing, it works pretty well. With ESM, all the JS now runs in strict mode which has potential to bring up new JS errors. Also I consolidated the entrypoint scripts to single files because this is required anyways for a future migration to Vite. jQuery.are-you-sure is moved to a vendor file because it is not compatible with strict mode.

I noticed Webpack had some issues with lazy-loading the PNG from monaco, and I concluded that it's not worth to maintain the js,css,fonts etc subdirectories, so I moved the assets to a single subdirectory static which is also what vite does and it eliminates this class of webpack errors.

This works quite well in my testing, but I'm putting WIP because I think there i a strict mode violation in bootstrap.js which is only triggered on error, so I'll investigate that later.

silverwind · 2023-07-17T23:01:55Z

This has an interaction with #25907, so that one should merge first after which I will move the output directory to /assets/static.

web_src/js/bootstrap.js

silverwind · 2023-07-18T16:07:09Z

Needs updates now that #25907 is in.

* main: Refactor "Content" for file uploading (go-gitea#25851) Fix SSPI auth panic (go-gitea#25955) Make pending commit status yellow again (go-gitea#25935) Move public asset files to the proper directory (go-gitea#25907) Disallow dangerous url schemes (go-gitea#25960) Avoid creating directories when loading config (go-gitea#25944) [skip ci] Updated translations via Crowdin

silverwind · 2023-07-18T21:45:58Z

This is ready. I could not reproduce the strict mode violation that I previously saw with vite, so maybe it was something vite-specific.

Makefile

silverwind · 2023-07-20T22:04:36Z

The strict mode-related error currently shows up on /devtest/gitea-ui:

@wxiaoguang if you have any suggestions to fix it and improve this hacky error handler, please go ahead. We really shouldn't need to fake an Array like that. My first idea is to use a Proxy, but I'm sure there are even simpler solutions.

wxiaoguang · 2023-07-21T04:06:11Z

Some code like initGlobalErrorHandler is executed more than once. (not tested yet, just a guess)

web_src/css/index.css

silverwind · 2023-07-29T13:42:28Z

WIP again because of #25940 (comment). Maybe we can't actually do ESM for the webcomponents because of that flicker.

silverwind · 2023-07-29T13:58:37Z

178538c appears to work to avoid the flicker. I do realize it's not ideal to load a module script as non-module, but it should work as long as we don't do import() inside the webcomponents chunk, which would be bad anyways because it again would be a source of flicker.

silverwind · 2023-07-29T14:04:56Z

Found a way to extract the devtest css again, so nothing from devtest should load in prod build again.

silverwind · 2023-07-30T00:49:31Z

As per this, combining type=module with async may be worth a try to get the webcomponents script to execute in a render-blocking way, I need to try.

silverwind · 2023-08-11T08:50:22Z

There is another consideration with type=module scripts from MDN:

Unlike classic scripts, module scripts require the use of the CORS protocol for cross-origin fetching

This would introduce additional issues for people who host assets on another domain. While I discourage this as it's unnecessary since HTTP2, it's a breaking change to them.

I have a feeling this change of script type is not worth it, at least not yet.

wxiaoguang · 2023-08-11T10:25:53Z

This would introduce additional issues for people who host assets on another domain. While I discourage this as it's unnecessary since HTTP2, it's a breaking change to them.

I have a feeling this change of script type is not worth it, at least not yet.

Then I guess it breaks STATIC_URL_PREFIX and Set `[server] STATIC_URL_PREFIX = http:https://cdn.example.com/gitea` in your configuration., and Gitea won't work with CDN?

silverwind · 2023-08-11T10:51:31Z

It could only work if said CDN would emit CORS headers, e.g. Access-Control-Allow-Origin. Still, I would never recommend serving assets on a different domain when HTTP2+ is in use, but I know there might be existing users that still do this.

If offloading the gitea web server from asset requests is the goal, the /assets/ subdirectory can be handled by the reverse proxy, and everything remains same-origin and browser security is happy. This is how I always deal with CORS, just don't do it, handle it at reverse proxy level instead.

silverwind · 2023-08-25T06:35:44Z

If this does not land, we should use esbuild banner feature to run all JS in strict mode: evanw/esbuild#422 (comment)

wxiaoguang · 2023-08-25T06:40:56Z

I haven't tested or fully understood the CDN problem. If it breaks CDN behavior, it would be a problem for many instances which depend on CDN ...... sorry I can't comment at the moment.

(ps: CDN is not just for a "different domain", it also for network speed, cost, server bandwidth, etc)

silverwind · 2023-08-25T06:44:57Z

I think long-term, we eventually have to move to ESM script output to remove this unnecessary translation to CommonJS that webpack does. The question is just "when", not "if" imho.

Maybe we should wait til the ESM feature stabilizes in webpack, but I imagine it won't be until webpack 6, if that ever comes.

Extract from #25940. `assetUrlPrefix` is guaranteed to not contain trailing slashes, making this function unneeded.

Extract from #25940 and because #26743 does seem to need more work. This will be required if we are to run our JS in [strict mode](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode). Previously, the two variables `$fields` and `$dirtyForms` polluted `window`: <img width="1145" alt="image" src="https://github.com/go-gitea/gitea/assets/115237/e0270a0e-b881-4ed7-9cc4-e9ab25c0a2bc">

silverwind · 2023-09-07T18:49:24Z

Not going to continue here for now. Maybe another try later.

Enable Webpack ESM output and move to /static/ asset directory

925a3f3

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 17, 2023

pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 17, 2023

fix WEBPACK_DEST

564361e

silverwind marked this pull request as draft July 17, 2023 23:15

silverwind added 2 commits July 18, 2023 01:21

fix WEBPACK_DEST_ENTRIES

098a72c

move import

e42d45a

silverwind commented Jul 18, 2023

View reviewed changes

web_src/js/bootstrap.js Outdated Show resolved Hide resolved

Update web_src/js/bootstrap.js

67a46fd

silverwind marked this pull request as ready for review July 18, 2023 21:45

devtest fixes

f4b9927

silverwind mentioned this pull request Jul 18, 2023

Build frontend with Vite #25874

Closed

wxiaoguang reviewed Jul 19, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

output assets directly to /assets/

6ab4848

silverwind changed the title ~~Enable Webpack ESM output and move to /static/ asset directory~~ Enable Webpack ESM output and move assets to /assets/ without subdirectories Jul 20, 2023

silverwind added 4 commits July 20, 2023 23:54

remove WEBPACK_DEST_ENTRIES

167c0d0

add back webpack asset cleaning in clean-all

1fa9115

use eval for performance

010607c

load bootstrap in devtest as well

9d6097a

Merge branch 'main' into static

8aefb87

silverwind marked this pull request as draft July 20, 2023 22:05

silverwind commented Jul 21, 2023

View reviewed changes

web_src/css/index.css Show resolved Hide resolved

silverwind added 2 commits July 29, 2023 13:29

Merge branch 'main' into static

5ccb1d4

use Proxy

174342b

silverwind marked this pull request as draft July 29, 2023 13:41

load webcomponents as non-module

178538c

silverwind force-pushed the static branch from ca94642 to 178538c Compare July 29, 2023 13:55

add comment

ee3484d

silverwind added 2 commits July 29, 2023 15:59

update comment

0ede200

use separate chunk for devtest css

48ca34c

silverwind marked this pull request as ready for review July 29, 2023 14:04

silverwind marked this pull request as draft August 11, 2023 08:47

silverwind added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Aug 11, 2023

silverwind mentioned this pull request Aug 20, 2023

Check disabled workflow when rerun jobs #26535

Merged

This was referenced Aug 26, 2023

Run JS in strict mode, vendorize jquery.are-you-sure #26742

Closed

Remove joinPaths function #26833

Merged

wxiaoguang pushed a commit that referenced this pull request Aug 31, 2023

Remove joinPaths function (#26833)

c35b16a

Extract from #25940. `assetUrlPrefix` is guaranteed to not contain trailing slashes, making this function unneeded.

silverwind mentioned this pull request Sep 4, 2023

Vendor jquery.are-you-sure with strict mode fixes #26901

Merged

silverwind closed this Sep 7, 2023

go-gitea locked as resolved and limited conversation to collaborators Dec 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Webpack ESM output and move assets to `/assets/` without subdirectories #25940

Enable Webpack ESM output and move assets to `/assets/` without subdirectories #25940

silverwind commented Jul 17, 2023 •

edited

Loading

silverwind commented Jul 17, 2023

silverwind commented Jul 18, 2023

silverwind commented Jul 18, 2023

silverwind commented Jul 20, 2023 •

edited

Loading

wxiaoguang commented Jul 21, 2023 •

edited

Loading

silverwind commented Jul 29, 2023

silverwind commented Jul 29, 2023

silverwind commented Jul 29, 2023

silverwind commented Jul 30, 2023

silverwind commented Aug 11, 2023

wxiaoguang commented Aug 11, 2023

silverwind commented Aug 11, 2023 •

edited

Loading

silverwind commented Aug 25, 2023 •

edited

Loading

wxiaoguang commented Aug 25, 2023 •

edited

Loading

silverwind commented Aug 25, 2023 •

edited

Loading

silverwind commented Sep 7, 2023

Enable Webpack ESM output and move assets to /assets/ without subdirectories #25940

Enable Webpack ESM output and move assets to /assets/ without subdirectories #25940

Conversation

silverwind commented Jul 17, 2023 • edited Loading

silverwind commented Jul 17, 2023

silverwind commented Jul 18, 2023

silverwind commented Jul 18, 2023

silverwind commented Jul 20, 2023 • edited Loading

wxiaoguang commented Jul 21, 2023 • edited Loading

silverwind commented Jul 29, 2023

silverwind commented Jul 29, 2023

silverwind commented Jul 29, 2023

silverwind commented Jul 30, 2023

silverwind commented Aug 11, 2023

wxiaoguang commented Aug 11, 2023

silverwind commented Aug 11, 2023 • edited Loading

silverwind commented Aug 25, 2023 • edited Loading

wxiaoguang commented Aug 25, 2023 • edited Loading

silverwind commented Aug 25, 2023 • edited Loading

silverwind commented Sep 7, 2023

Enable Webpack ESM output and move assets to `/assets/` without subdirectories #25940

Enable Webpack ESM output and move assets to `/assets/` without subdirectories #25940

silverwind commented Jul 17, 2023 •

edited

Loading

silverwind commented Jul 20, 2023 •

edited

Loading

wxiaoguang commented Jul 21, 2023 •

edited

Loading

silverwind commented Aug 11, 2023 •

edited

Loading

silverwind commented Aug 25, 2023 •

edited

Loading

wxiaoguang commented Aug 25, 2023 •

edited

Loading

silverwind commented Aug 25, 2023 •

edited

Loading