Use npm to manage fomantic and only build needed components #9561

lunny · 2019-12-31T06:02:52Z

Removed semantic.min.css from git and use npm to build fomantic.
Only load needed semantic components. Before this PR, semantic.min.css is 1.29MB, after ~650KB.
Don't load fonts from google sites since it's slow from some location visit. And the lato fonts has been loaded by gitea itself.

codecov-io · 2020-01-01T07:11:24Z

Codecov Report

Merging #9561 into master will decrease coverage by 0.03%.
The diff coverage is 11.73%.

@@            Coverage Diff             @@
##           master    #9561      +/-   ##
==========================================
- Coverage   42.33%   42.29%   -0.04%     
==========================================
  Files         605      607       +2     
  Lines       79313    79353      +40     
==========================================
- Hits        33577    33565      -12     
- Misses      41597    41645      +48     
- Partials     4139     4143       +4

Impacted Files	Coverage Δ
models/repo.go	`49.76% <ø> (+2.73%)`	⬆️
modules/repository/repo.go	`26.28% <0%> (ø)`	⬆️
models/context.go	`39.28% <0%> (-6.55%)`	⬇️
services/wiki/wiki.go	`57.08% <0%> (ø)`	⬆️
cmd/admin.go	`0% <0%> (ø)`	⬆️
routers/admin/admin.go	`11.2% <0%> (-0.04%)`	⬇️
models/migrations/v36.go	`0% <0%> (ø)`	⬆️
modules/repository/init.go	`57.43% <0%> (ø)`	⬆️
modules/cron/cron.go	`47.19% <0%> (-4%)`	⬇️
modules/repository/check.go	`0% <0%> (ø)`
... and 10 more

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 7818e13...b11bdba. Read the comment docs.

bagasme

Huge PR, right?

lunny · 2020-01-01T10:25:07Z

@bagasme I don't think so. Most changes are to delete semantic-ui vendor files and add semantic-ui sources.

lafriks · 2020-01-02T00:35:40Z

package.json

+  ],
+  "dependencies": {
+    "fomantic-ui": "2.8.3",
+    "gulp": "4.0.2"


Gulp must be devDependency

Can't we just use webpack without gulp?

@lafriks I just follow https://fomantic-ui.com/introduction/build-tools.html to use gulp. If we want to use webpack, I think we should do that another PRs.

Gulp build is necessary to build the fomantic bundle I'm afraid, but we can tweak the Makefile so it only runs when necessary. Optionally, the resulting files could then be integrated in the webpack bundle as most pages do need it (thought, not all).

lunny · 2020-01-02T03:28:55Z

@lafriks Done. I also moved fomantic-ui to devDependencies.

6543 · 2020-01-02T10:44:25Z

@lunny is there a optio to only build fomatic if something has changed?
-> as we do with webpack
it is build every time you build gitea witch is anoing

lafriks · 2020-01-02T12:25:44Z

Fomatic-ui should be dependency, and gulp devDependency

package.json

silverwind · 2020-01-04T02:25:56Z

Makefile

+.PHONY: fomantic
+fomantic: node-check node_modules
+	cd web_src/fomantic && npx gulp build
+


Maybe it is possible to have this as a non-phony target that depends on package-lock.json, so it would only run if dependencies change.

Also, I'd suggest the target to be adjusted to provide one or more output files, like public/fomantic/semantic.min.js and public/fomantic/semantic.min.css. We can still have a phony fomantic target that depends on that file to trigger it on-demand.

semantic.json

das7pad

@lunny @6543 I managed to devendor fomantic (8MB on disk) and implement the lazy build.

Feel free to cherry pick das7pad@6a012e9

package.json

lunny · 2020-01-07T07:03:11Z

@das7pad Thanks! @lafriks @silverwind done.

silverwind · 2020-01-07T12:19:07Z

Please remove the gulp dependency.

Not sure I like the approach of writing into node_modules, I think it may be better to do it outside and treat it as read-only.

das7pad · 2020-01-07T12:30:30Z

Please remove the gulp dependency.

Can you help me understand why it should be removed?

Not sure I like the approach of writing into node_modules, I think it may be better to do it outside and treat it as read-only.

I do not like it either, but their imports dictate that this config file is placed in the source code:
https://github.com/fomantic/Fomantic-UI/blob/037a0402362fa2ffa44e434fe7e362c8cb753bb8/src/definitions/globals/reset.less#L18

As noted in the TODO, we can work around that with an alias/mock, but I could not find an easy solution for that using the gulp build tool. We could fix the fomantic source too, but i'm not sure it is worth the effort.

Makefile

silverwind · 2020-01-07T16:33:55Z

Can you help me understand why it should be removed?

Because it already comes in as a dependency of fomantic. If we depend on a specific version of it directly and fomantic updates their version, we'd end up with two versions which is not ideal (not sure which one would end up in the .bin scripts in that case).

If you really want to keep it, I'd suggest depending on version * so the more specific definition in fomantic would take precedence.

das7pad · 2020-01-07T17:47:41Z

Thanks @silverwind !

Can you help me understand why it should be removed?

Because it already comes in as a dependency of fomantic. If we depend on a specific version of it directly and fomantic updates their version, we'd end up with two versions which is not ideal (not sure which one would end up in the .bin scripts in that case).

If you really want to keep it, I'd suggest depending on version * so the more specific definition in fomantic would take precedence.

Our version would go into .bin, theirs in their node_modules.
I do not think that having two versions is a problem per se. gulp4s' cli could be perfectly compatible with the gulp5 modules that their build-file loads. But gulp5 could also drop the -f option or expect a sub command to trigger the full build process. I would say the it is up to the developer who proposes a new fomantic (or gulp) version to make sure it is compatible with the build tooling.

I agree that we may be better off with a more loose version. ^4.0.0 would leave full room for minor/patch version upgrades but retain the major version at 4. IIRC * does not play nice with lock files and we do not want to end up with gulp9 when fomantic is stuck at 4.

silverwind · 2020-01-09T12:52:25Z

In the end, it doesn't really matter much because we already lock dependencies. Still, I would recommend removing it from our dependencies, the fomantic docs also don't recommend adding gulp to the project dependencies.

package.json

lafriks · 2020-01-19T11:57:06Z

gulp should be added as devDependency

lunny · 2020-01-19T14:02:57Z

In the end, it doesn't really matter much because we already lock dependencies. Still, I would recommend removing it from our dependencies, the fomantic docs also don't recommend adding gulp to the project dependencies.

@lafriks as @silverwind mentioned. fomantic-ui has a dependency on gulp already.

silverwind · 2020-01-20T08:42:19Z

@lafriks we discussed this a few times already. Given that gulp comes as a second-level dependency, I think it's better to not directly depend on it to allow fomantic-ui choose their appropriate version.

Signed-off-by: Jakob Ackermann <das7pad@outlook.com>

silverwind · 2020-01-22T20:25:24Z

@lunny: any specific reason why you added fomantic as prereq to js and css steps? I would prefer to be able to run webpack without fomantic building.

lunny · 2020-01-23T03:22:33Z

@silverwind I just thought, maybe some js or css files depend on fomantic since it's a basic framework. So I did that. If that's not a thing, I think we can change that.

lunny added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Dec 31, 2019

lunny mentioned this pull request Jan 1, 2020

Custom fonts URL on fomantic #9562

Closed

lunny force-pushed the lunny/fomantic_npm branch from 80e7a6f to 670cb9a Compare January 1, 2020 06:00

lunny changed the title ~~Use npm to manage fomantic~~ Use npm to manage fomantic and only build needed components Jan 1, 2020

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

bagasme reviewed Jan 1, 2020

View reviewed changes

lafriks requested changes Jan 2, 2020

View reviewed changes

lunny force-pushed the lunny/fomantic_npm branch from 5ea0338 to 9bcd49d Compare January 2, 2020 03:25

sapk added this to the 1.12.0 milestone Jan 2, 2020

silverwind reviewed Jan 4, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

silverwind reviewed Jan 4, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

silverwind reviewed Jan 4, 2020

View reviewed changes

semantic.json Outdated Show resolved Hide resolved

silverwind reviewed Jan 4, 2020

View reviewed changes

semantic.json Show resolved Hide resolved

das7pad reviewed Jan 6, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

lunny force-pushed the lunny/fomantic_npm branch from 9bcd49d to 9830520 Compare January 7, 2020 07:00

das7pad reviewed Jan 7, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

lunny force-pushed the lunny/fomantic_npm branch from d5bf2ec to e4c3ed7 Compare January 7, 2020 15:47

lunny force-pushed the lunny/fomantic_npm branch from e4c3ed7 to 2b10c76 Compare January 9, 2020 13:17

silverwind reviewed Jan 16, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

lunny force-pushed the lunny/fomantic_npm branch from 9df17aa to a73a90e Compare January 17, 2020 00:55

lafriks approved these changes Jan 19, 2020

View reviewed changes

lunny and others added 11 commits January 20, 2020 23:12

Use npm to manage fomantic

ca23114

Only build needed semantic components

d195d0d

Fix make

a56ee78

Don't import fonts from google sites since we have loaded

0ee3b2b

[misc] devendor fomantic-ui and rebuild upon src or config changes only

eb3c9ea

Signed-off-by: Jakob Ackermann <das7pad@outlook.com>

Change sort alphabetically of semantic components

123ee04

Fix trailing slash

07d0b71

fix makefile

1764412

Remove dependency to gulp from package.json

627bd16

Fix something

02d0f79

Simplife the makefile

b3013b1

lunny force-pushed the lunny/fomantic_npm branch from ac566a3 to b3013b1 Compare January 20, 2020 15:14

lunny added 3 commits January 20, 2020 23:17

add missed fomantic compnent

4f30548

Merge branch 'master' into lunny/fomantic_npm

7818e13

Merge branch 'master' into lunny/fomantic_npm

b11bdba

lunny merged commit 5cf241b into go-gitea:master Jan 21, 2020

lunny deleted the lunny/fomantic_npm branch January 21, 2020 05:18

silverwind mentioned this pull request Jan 21, 2020

Custom fomantic-ui build #9415

Closed

lunny mentioned this pull request Jan 21, 2020

Fix wrong font url on semantic-ui #9917

Merged

alexanderadam mentioned this pull request Apr 29, 2020

UI loading google fonts for lato #11243

Closed

CirnoT mentioned this pull request May 12, 2020

Add missing card component to semantic.json #11394

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use npm to manage fomantic and only build needed components #9561

Use npm to manage fomantic and only build needed components #9561

lunny commented Dec 31, 2019 •

edited

codecov-io commented Jan 1, 2020 •

edited

bagasme left a comment

lunny commented Jan 1, 2020

lafriks Jan 2, 2020

lafriks Jan 2, 2020

lunny Jan 2, 2020

silverwind Jan 4, 2020 •

edited

lunny commented Jan 2, 2020

6543 commented Jan 2, 2020 •

edited

lafriks commented Jan 2, 2020

silverwind Jan 4, 2020

silverwind Jan 4, 2020 •

edited

das7pad left a comment

lunny commented Jan 7, 2020

silverwind commented Jan 7, 2020

das7pad commented Jan 7, 2020

silverwind commented Jan 7, 2020 •

edited

das7pad commented Jan 7, 2020

silverwind commented Jan 9, 2020

lafriks commented Jan 19, 2020

lunny commented Jan 19, 2020

silverwind commented Jan 20, 2020

silverwind commented Jan 22, 2020

lunny commented Jan 23, 2020 •

edited

Use npm to manage fomantic and only build needed components #9561

Use npm to manage fomantic and only build needed components #9561

Conversation

lunny commented Dec 31, 2019 • edited

codecov-io commented Jan 1, 2020 • edited

Codecov Report

bagasme left a comment

Choose a reason for hiding this comment

lunny commented Jan 1, 2020

lafriks Jan 2, 2020

Choose a reason for hiding this comment

lafriks Jan 2, 2020

Choose a reason for hiding this comment

lunny Jan 2, 2020

Choose a reason for hiding this comment

silverwind Jan 4, 2020 • edited

Choose a reason for hiding this comment

lunny commented Jan 2, 2020

6543 commented Jan 2, 2020 • edited

lafriks commented Jan 2, 2020

silverwind Jan 4, 2020

Choose a reason for hiding this comment

silverwind Jan 4, 2020 • edited

Choose a reason for hiding this comment

das7pad left a comment

Choose a reason for hiding this comment

lunny commented Jan 7, 2020

silverwind commented Jan 7, 2020

das7pad commented Jan 7, 2020

silverwind commented Jan 7, 2020 • edited

das7pad commented Jan 7, 2020

silverwind commented Jan 9, 2020

lafriks commented Jan 19, 2020

lunny commented Jan 19, 2020

silverwind commented Jan 20, 2020

silverwind commented Jan 22, 2020

lunny commented Jan 23, 2020 • edited

lunny commented Dec 31, 2019 •

edited

codecov-io commented Jan 1, 2020 •

edited

silverwind Jan 4, 2020 •

edited

6543 commented Jan 2, 2020 •

edited

silverwind Jan 4, 2020 •

edited

silverwind commented Jan 7, 2020 •

edited

lunny commented Jan 23, 2020 •

edited