Avoid unnecessary system-ui expansion #12522

CL-Jeremy · 2020-08-18T12:47:42Z

cf. #11818 regarding emojis.

silverwind · 2020-08-18T13:00:41Z

webpack.config.js

+  features: {
+    'system-ui-font-family': false,
+  }
+});


I think we can put this config into the postcss-loader options. That way, it'll be easier to convert this file to ESM syntax later. Keep note that there are two such sections, one for CSS and one for Less.

{ loader: 'postcss-loader', options: { sourceMap: true, plugins: () => [ PostCSSPresetEnv({ features: { 'system-ui-font-family': false, }, }), ], }, },

I was about to do this:

const PostCSSPresetEnv = (args) => { args = {...args}; return require('postcss-preset-env')({...args, features: {...args.features, 'system-ui-font-family': false, } }); };

I'm not sure about the link between ESM syntax and having this at two places.

With ESM one would convert

const PostCSSPresetEnv = require('postcss-preset-env');

to

import PostCSSPresetEnv from 'postcss-preset-env';

meaning the import statement has to stand alone, so I think it's better to leave it a simple require statement. You can optionally define the config on the top-level scope in a variable and reference that further down, but I don't see the duplication a big issue.

CL-Jeremy@a74795d

Nice, didn't know that trick. The only thing that worries me a bit is that esm seems to lean a bit on the unmaintained side, but I guess we can eventually remove this workaround once webpack supports node's ESM implementation.

Change looks good besides this. Can you either replicate the postCSSPlugins variable usage in this PR or alternatively also merge the ESM conversion into here?

codecov-commenter · 2020-08-18T13:31:03Z

Codecov Report

Merging #12522 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #12522      +/-   ##
==========================================
- Coverage   43.44%   43.42%   -0.02%     
==========================================
  Files         645      645              
  Lines       71328    71328              
==========================================
- Hits        30986    30977       -9     
- Misses      35331    35336       +5     
- Partials     5011     5015       +4

Impacted Files	Coverage Δ
modules/indexer/stats/db.go	`52.17% <0.00%> (-8.70%)`	⬇️
modules/git/utils.go	`73.77% <0.00%> (-3.28%)`	⬇️
modules/process/manager.go	`72.50% <0.00%> (-2.50%)`	⬇️
modules/git/repo.go	`49.23% <0.00%> (-0.51%)`	⬇️
services/pull/pull.go	`41.57% <0.00%> (-0.47%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5b6931...8688513. Read the comment docs.

silverwind · 2020-08-18T20:38:57Z

I think you can use -r @babel/register for the same effect. It looks to be more active than esm.

Oh, and please run npx eslint --fix webpack.config.js.

CL-Jeremy · 2020-08-18T22:27:01Z

I think you can use -r @babel/register for the same effect. It looks to be more active than esm.

~~Actually, both are needed, cf. webpack/webpack-cli#282 (comment).~~

Babel alone doesn't offer drop-in support of ESM entry point for Node.js. They target different goals and are complementary in this case. I'd even say there isn't much to be done for ESM with the effort of all the projects in the ecosystem toward modern ECMAScript support. The leading developer of esm has been quite active in helping with issues related to this topic, so it isn't like the project is abandoned or so. While this is still true, @babel/register does work as a drop-in replacement of esm. I just didn't know I had to add:

  "babel": {
    "presets": [
      "@babel/preset-env"
    ]
  }

~~As there's no cutting-edge ECMAScript feature involved now, I'm not including that here. It'll definitely be useful to have ??, ?., etc. Also note the extra dependencies involved.~~
The best solution would be to step up Node.js version requirement and get native support of ECMAScript features. That'll be a treat.

silverwind · 2020-08-19T05:05:40Z

Strangely enough, @babel/register did work for me without the babel config, but I guess it does not hurt to have it.

silverwind

Actually I think it may be better to move the babel config to a .babelrc.json so we only define it in one place for the whole project. Will test this later, blocking for now.

silverwind · 2020-08-19T15:48:13Z

I was not able to get @babel/register to work on my Windows machine so I think we should switch back to using esm. You can pull in silverwind@f979cb9, then I'm happy.

CL-Jeremy · 2020-08-21T01:06:29Z

Built on latest MSYS2 with packaged Go and official Node.js 14.8.0.
gitea.exe.92bf4e77.xz.gz

silverwind · 2020-08-21T13:59:03Z

It was giving me Cannot use import outside module errors. Maybe something specific to my setup (Cygwin).

BTW: I never requested this PR to do the ESM conversion so if you like, you can turn the imports back into require and just keep the change this was intended to fix. Probably better to use ESM once webpack.config.mjs is officially supported.

CL-Jeremy · 2020-08-21T14:36:53Z

How is Gitea officially built on Windows? By cross compilation or natively?

I really don't think Cygwin is the source of the problem. Sounds more like path misconfiguration with Node.js.

And I really think MSYS2 is just wonderful, aside from the not-so-great shell GUI (giving me broken encoding) which is replaced by the also wonderful Windows Terminal here. CI integration is also possible (though I don't see any benefit from that apart from ensuring the build setup works for those who are willing to build on Windows).

silverwind · 2020-08-21T14:42:22Z

How is Gitea officially built on Windows? By cross compilation or natively?

Official binaries are cross-compiled. It should theoretically be possible to build on Windows natively when make and the unix tools used in the Makefile are available.

I really don't think Cygwin is the source of the problem. Sounds more like path misconfiguration with Node.js.

I tried the webpack command in cmd and it didn't work either. Once I switched to -r esm, it worked instantly. Might be some obscure path issue with the @ or / but nothing I've tried worked. On the other hand, macOS worked fine with both preloaders. Anyways, I think we better revert to CJS for now.

silverwind · 2020-08-21T14:46:53Z

Found the issue. It's babel/babel#10232 (comment). My parent folder is a junction and it works after doing it in the actual destination directory. I don't think such workaround are acceptable, it's certainly a bug in the module.

CL-Jeremy · 2020-08-21T17:19:55Z

Anyways, I think we better revert to CJS for now.

In this case, I believe it's commonplace to chain/call or even do whatever it's appropriate to the actual require() clause. I prefer to just leave it like my initial commit.

I'm actually testing swc-loader and @swc/register as a drop-in replacement to Babel. It now supports ES2020 which is pretty nice.

CL-Jeremy · 2020-08-21T18:40:54Z

Just tested with swc. Source map generation was presumably the bottleneck, and now @swc/register didn't work for me, either (so I tested on macOS, dual-core i5).

Yeah, pretty frustrating, especially when considering its performance per se and TypeScript support.

silverwind · 2020-08-21T18:44:18Z

Plain Node.js will always be fastest so I prefer to use that until we can use native module syntax.

In this case, I believe it's commonplace to chain/call or even do whatever it's appropriate to the actual require() clause. I prefer to just leave it like my initial commit.

I guess I don't really care. I've been personally avoiding doing anything directly on the same line as require lately because those construts make migration to ESM slightly harder than it needs to be.

CL-Jeremy · 2020-08-21T19:05:55Z

Oh, and here's the usage of swc in case it becomes suitable in the future: 5c913da

Plain Node.js will always be fastest so I prefer to use that until we can use native module syntax.

No, I'm actually replacing the webpack part, which becomes slower due to source map generation. The improvement in compilation time of webpack.config.js is of course negligible.

In the context of Node.js, the other modules with Babel dependencies are actually what may benefit from the performance boost (they claim >= 18x) from swc. That's of course way beyond our scope, though.

silverwind · 2020-08-21T19:21:46Z

Faster transpilation is always welcome. I assume we soon won't need to transpile to ES5 (which last I checked is still happening) and can target more modern browsers which should reduce JS bundle size a bit (I've read it's around 30% less with ES5 polyfills gone).

silverwind · 2020-08-21T20:55:12Z

Offtopic: I'm certainly watching swc but for use in gitea, we'd need to adjust the release process and stop publishing node_modules because it is a native code dependency and the src tarballs need to stay platform-independant.

silverwind · 2020-08-26T18:57:34Z

Sounds like you're a lot more knowledgeable about fonts than I am. I'm happy if you want to atttempt to resolve #11045 in another PR later. Getting one globally working font stack sure is not a easy task. Gitee is probably biased towards Chinese audiences, GitHub towards western.

silverwind · 2020-08-26T19:08:41Z

Would you mind pulling in silverwind@41abd41 here? I feel like this is more readable as the config block in the middle of the requires looks odd to me.

CL-Jeremy · 2020-08-26T19:33:31Z

I got shallow update not allowed again...
Okay, for the first time in my life I'm gonna just try to fetch --unshallow... It's super slow...

silverwind · 2020-08-26T19:42:59Z

This should be enough:

git remote add silverwind git@github.com:silverwind/gitea.git
git fetch silverwind postcssfix
git cherry-pick 41abd41f424b8cc0c82001575defcad53d2bc0df

Also, I strongly recommend not using master for pull requests 😉

CL-Jeremy · 2020-08-26T19:47:54Z

The thing is, I actually wanted to merge my two commits to they are at one place in the history.

I guess I'll just clone the whole thing again...

silverwind · 2020-08-26T19:52:31Z

git rebase -i origin/master (assuming origin is this repo) and mark the second one as fixup will squash reliably. Thought it's not required to squash, we do that on merge anyways.

CL-Jeremy · 2020-08-26T20:09:47Z

git rebase -i origin/master (assuming origin is this repo) and mark the second one as fixup will squash reliably. Thought it's not required to squash, we do that on merge anyways.

I think that'll squash it onto its parent. That's bad.
Anyway I just clone the whole thing on a remote at 25 MB/s and rsync'd it to local at 15 MB/s... previously 50 kB/s.... 😭

CL-Jeremy · 2020-08-26T20:18:56Z

Ohh now I see what happened here... by merging your commit I resurrected those buried commits since you forked that branch from me...

silverwind · 2020-08-26T20:45:06Z

Would probably have been easier to use GitHub's "secret" .patch url 😉

CL-Jeremy · 2020-08-26T21:01:48Z

Would probably have been easier to use GitHub's "secret" .patch url wink

Wow, as a relatively experienced Homebrew contributor I thought I'm fairly familiar with the existence of .patch and .diff. Good to know there's also git am. 👍

silverwind · 2020-08-27T07:39:51Z

web_src/less/_base.less

@@ -18,7 +18,7 @@
    url('../fonts/noto-color-emoji/NotoColorEmoji.ttf') format('truetype');
 }

-@default-fonts: -apple-system, BlinkMacSystemFont, system-ui, 'Segoe UI', Roboto, Helvetica, Arial, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol", "Noto Color Emoji" sans-serif;
+@default-fonts: -apple-system, BlinkMacSystemFont, system-ui, 'Segoe UI', Roboto, Helvetica, Arial, "Apple Color Emoji", "Segoe UI Emoji", "Noto Color Emoji", "Twemoji Mozilla";


Just noticed it: Is the removal of Segoe UI Symbol intentional?

Yes. I take it as something in the same league as JIS fonts and DejaVu Sans. As I said earlier, it's not a major goal to cater users with need of these black-and-white icons (except for code, but maybe even there as well). I'm removing it to align the UX of legacy Windows to legacy Linux, where DejaVu Sans is only matched by sans-serif. Testing it on Windows 7 shows the same effect with Segoe UI Symbol being matched by sans-serif, so at least there won't be tofus.

There have been discussions regarding this in the past when browsers switched to prioritize emojis, a behavior originated on mobile platforms, cf. https://bugzilla.mozilla.org/show_bug.cgi?id=1054780.

Another (positive?) side effect of this would be that Japanese users would see more native glyphs of dingbat symbols if they have compatible fonts in the :lang(ja) stack installed. emoticons, people, objects and stuff will be unaffected. If I recall correctly, many JIS symbol designs from non-Japanese fonts (e. g. emoji fonts) look poor and broken. I haven't investigated the Segoe ones though.

Sounds good. Agree that plain color glyphs should not be used if better versions are available.

…r-fix

CL-Jeremy · 2020-08-28T12:31:08Z

Had to force-push again to recover this lost change during the rebase:

gitea/web_src/less/_base.less

Line 82 in 8688513

.override-fonts(@default-fonts, sans-serif;);

silverwind · 2020-08-28T12:52:33Z

Still looking good. I think the semicolons inside the parents can be removed but I see they were partially there before this PR so it's fine either way.

CL-Jeremy · 2020-08-28T13:00:41Z

Still looking good. I think the semicolons inside the parents can be removed but I see they were partially there before this PR so it's fine either way.

That's what I tried before submitting this PR. Syntax error.

CL-Jeremy changed the title ~~Avoid unnecessary system-ui expansion (fix #12325)~~ Avoid unnecessary system-ui expansion Aug 18, 2020

silverwind reviewed Aug 18, 2020

View reviewed changes

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 18, 2020

CL-Jeremy force-pushed the master branch from a74795d to 398bc74 Compare August 18, 2020 20:47

CL-Jeremy force-pushed the master branch 2 times, most recently from 65b2b37 to 92bf4e7 Compare August 18, 2020 23:32

silverwind approved these changes Aug 19, 2020

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 19, 2020

silverwind requested changes Aug 19, 2020

View reviewed changes

CL-Jeremy force-pushed the master branch from 92bf4e7 to 394f247 Compare August 21, 2020 18:43

silverwind approved these changes Aug 21, 2020

View reviewed changes

zeripath added this to the 1.13.0 milestone Aug 22, 2020

zeripath added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Aug 22, 2020

CL-Jeremy force-pushed the master branch from 142b472 to 198cc56 Compare August 26, 2020 19:06

CL-Jeremy force-pushed the master branch from 198cc56 to 9a1e40d Compare August 26, 2020 20:16

CL-Jeremy force-pushed the master branch from 9a1e40d to 84fb471 Compare August 26, 2020 20:39

silverwind approved these changes Aug 26, 2020

View reviewed changes

silverwind reviewed Aug 27, 2020

View reviewed changes

CL-Jeremy and others added 3 commits August 28, 2020 14:20

Avoid unnecessary system-ui expansion (fix go-gitea#12325)

b721822

extract config to static object

0627cab

Merge branch 'master' of https://github.com/go-gitea/gitea into maste…

8688513

…r-fix

CL-Jeremy force-pushed the master branch from 84fb471 to 8688513 Compare August 28, 2020 12:26

Merge branch 'master' into master

0f9a113

techknowlogick approved these changes Aug 28, 2020

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 28, 2020

techknowlogick merged commit e1eee2d into go-gitea:master Aug 28, 2020

go-gitea locked and limited conversation to collaborators Nov 24, 2020

CL-Jeremy deleted the master branch February 4, 2021 20:53

Avoid unnecessary system-ui expansion #12522

Avoid unnecessary system-ui expansion #12522

Conversation

CL-Jeremy commented Aug 18, 2020

silverwind Aug 18, 2020 • edited

Choose a reason for hiding this comment

CL-Jeremy Aug 18, 2020

Choose a reason for hiding this comment

silverwind Aug 18, 2020 • edited

Choose a reason for hiding this comment

CL-Jeremy Aug 18, 2020

Choose a reason for hiding this comment

silverwind Aug 18, 2020 • edited

Choose a reason for hiding this comment

silverwind Aug 18, 2020

Choose a reason for hiding this comment

codecov-commenter commented Aug 18, 2020 • edited

Codecov Report

silverwind commented Aug 18, 2020 • edited

CL-Jeremy commented Aug 18, 2020 • edited

silverwind commented Aug 19, 2020 • edited

silverwind left a comment • edited

Choose a reason for hiding this comment

silverwind commented Aug 19, 2020 • edited

CL-Jeremy commented Aug 21, 2020 • edited

silverwind commented Aug 21, 2020 • edited

CL-Jeremy commented Aug 21, 2020 • edited

silverwind commented Aug 21, 2020

silverwind commented Aug 21, 2020 • edited

CL-Jeremy commented Aug 21, 2020

CL-Jeremy commented Aug 21, 2020

silverwind commented Aug 21, 2020 • edited

CL-Jeremy commented Aug 21, 2020 • edited

silverwind commented Aug 21, 2020

silverwind commented Aug 21, 2020 • edited

silverwind commented Aug 26, 2020

silverwind commented Aug 26, 2020

CL-Jeremy commented Aug 26, 2020 • edited

silverwind commented Aug 26, 2020 • edited

CL-Jeremy commented Aug 26, 2020

silverwind commented Aug 26, 2020

CL-Jeremy commented Aug 26, 2020

CL-Jeremy commented Aug 26, 2020

silverwind commented Aug 26, 2020

CL-Jeremy commented Aug 26, 2020

silverwind Aug 27, 2020

Choose a reason for hiding this comment

CL-Jeremy Aug 27, 2020

Choose a reason for hiding this comment

CL-Jeremy Aug 27, 2020

Choose a reason for hiding this comment

silverwind Aug 27, 2020

Choose a reason for hiding this comment

CL-Jeremy commented Aug 28, 2020

silverwind commented Aug 28, 2020

CL-Jeremy commented Aug 28, 2020

silverwind Aug 18, 2020 •

edited

silverwind Aug 18, 2020 •

edited

silverwind Aug 18, 2020 •

edited

codecov-commenter commented Aug 18, 2020 •

edited

silverwind commented Aug 18, 2020 •

edited

CL-Jeremy commented Aug 18, 2020 •

edited

silverwind commented Aug 19, 2020 •

edited

silverwind left a comment •

edited

silverwind commented Aug 19, 2020 •

edited

CL-Jeremy commented Aug 21, 2020 •

edited

silverwind commented Aug 21, 2020 •

edited

CL-Jeremy commented Aug 21, 2020 •

edited

silverwind commented Aug 21, 2020 •

edited

silverwind commented Aug 21, 2020 •

edited

CL-Jeremy commented Aug 21, 2020 •

edited

silverwind commented Aug 21, 2020 •

edited

CL-Jeremy commented Aug 26, 2020 •

edited

silverwind commented Aug 26, 2020 •

edited