Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support NODE_ENV in webpack #10245

Merged
merged 4 commits into from
Feb 23, 2020
Merged

Support NODE_ENV in webpack #10245

merged 4 commits into from
Feb 23, 2020

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Feb 11, 2020

  • Set webpack to development mode when NODE_ENV=development.

@silverwind silverwind changed the title Add new webpack-dev target for frontend development Add 'make webpack-dev' for frontend development Feb 11, 2020
@silverwind
Copy link
Member Author

make run added which should resolve the Windows issue.

make did require a special incantation to be able to pass arguments to the executable, so I added that as well, so we can now make run -- --help.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 12, 2020
@silverwind
Copy link
Member Author

silverwind commented Feb 12, 2020

Simplified so we can now use parallel make jobs to run both targets in parallel:

make build && make -j2 run webpack-dev

Of course, users are free to start gitea however they like and run make webpack-dev separately.

@codecov-io
Copy link

codecov-io commented Feb 12, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10245      +/-   ##
==========================================
- Coverage   43.71%   43.69%   -0.03%     
==========================================
  Files         586      586              
  Lines       81351    81351              
==========================================
- Hits        35565    35547      -18     
- Misses      41389    41404      +15     
- Partials     4397     4400       +3
Impacted Files Coverage Δ
services/pull/check.go 50% <0%> (-5.49%) ⬇️
modules/git/command.go 86.95% <0%> (-2.61%) ⬇️
services/pull/temp_repo.go 29.05% <0%> (-2.57%) ⬇️
services/pull/patch.go 60.37% <0%> (-2.52%) ⬇️
models/notification.go 63.29% <0%> (-1.9%) ⬇️
modules/indexer/stats/db.go 50% <0%> (ø) ⬆️
modules/git/repo.go 47.7% <0%> (+0.91%) ⬆️
modules/log/event.go 65.64% <0%> (+1.02%) ⬆️
modules/process/manager.go 78.31% <0%> (+3.61%) ⬆️
modules/git/utils.go 70.14% <0%> (+4.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 ad2b913...ad21c4f. Read the comment docs.

@silverwind
Copy link
Member Author

There is a issue with the run argument filtering make -j2 run webpack-dev --port 4000 runs gitea with ./gitea webpack-dev --port 4000 and it seems gitea ignores flags like --port when the command to start is unknown (but the server is still started up, so probably a bug in itself in the args parser). Will check later if that argument filtering mechanism can be fixed.

@silverwind
Copy link
Member Author

Maybe I'll just change it so a simple make webpack-dev does everything including the sub-makes to build and parallel run/watch.

@silverwind silverwind changed the title Add 'make webpack-dev' for frontend development WIP: Add 'make webpack-dev' for frontend development Feb 12, 2020
@silverwind
Copy link
Member Author

Marking as WIP for now. I'm still looking for a better solution to start the gitea server in the background for webpack development.

@lafriks
Copy link
Member

lafriks commented Feb 13, 2020

It would be better to add code to Gitea that would start webpack watch in background with special flag

@silverwind silverwind changed the title WIP: Add 'make webpack-dev' for frontend development Add 'make webpack-dev' Feb 13, 2020
@silverwind
Copy link
Member Author

@lafriks yes, that would be ideal, but it's probably not within my abilities.

Reduced the changes in this PR so it's not only about adding webpack-dev and a docs update, so I think it is good to go.

webpack.config.js Outdated Show resolved Hide resolved
@silverwind
Copy link
Member Author

silverwind commented Feb 14, 2020

Source maps set on babel-loader as well just in case.

There seems to be an issue with the production source maps (I think browsers may try to retrieve them on the wrong URL) but it's unrelated to this PR and master is also affected.

@lafriks
Copy link
Member

lafriks commented Feb 21, 2020

@silverwind I created golang package that can be used for this built into Gitea binary: https://github.com/lafriks/go-spaproxy

@silverwind
Copy link
Member Author

@lafriks feel free to take over with another PR. Did you really get hot reloading to work? That'd be even more awesome than --watch which this PR uses.

@lafriks
Copy link
Member

lafriks commented Feb 21, 2020

Hot reloading will work only for spa apps but would be good if we could at least get watch included in Gitea binary so you would not have run multiple commands

@silverwind
Copy link
Member Author

Yeah, I assume as long as webpack is not in control of HTML, you can not really get hot reloading to work, at least not without major hacks. --watch should be fine for us as a start.

@silverwind silverwind changed the title Add 'make webpack-dev' Support NODE_ENV in webpack Feb 21, 2020
@silverwind
Copy link
Member Author

Removed the Makefile changes, so this PR is now only about supporting NODE_ENV=development which is universally useful.

@lafriks lafriks added topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Feb 22, 2020
@lafriks lafriks added this to the 1.12.0 milestone Feb 22, 2020
@GiteaBot 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 Feb 23, 2020
@GiteaBot 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 Feb 23, 2020
@lafriks lafriks merged commit 9ed4aee into go-gitea:master Feb 23, 2020
@silverwind silverwind deleted the devmode branch February 23, 2020 08:53
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants