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

move swagger-ui to webpack/npm and update it to 3.24.3 #9714

Merged
merged 1 commit into from Jan 14, 2020

Conversation

@silverwind
Copy link
Member

silverwind commented Jan 11, 2020

Created a second webpack output file for swagger-ui which is loaded on the /api/swagger route. One notable difference is the absence of the swagger favicon that was previously used which is now the gitea icon. I see no easy way to restore that favicon, so I decided to not keep it.

@silverwind

This comment has been minimized.

Copy link
Member Author

silverwind commented Jan 11, 2020

Also, this will update swagger-ui from 3.13.4 to 3.24.3.

@6543

This comment has been minimized.

Copy link
Member

6543 commented Jan 11, 2020

@silverwind can you add a screenshot
edit: thanks

@silverwind

This comment has been minimized.

Copy link
Member Author

silverwind commented Jan 12, 2020

Screenshot added, but it's kind of pointless because page content has not changed 😉

@6543

This comment has been minimized.

Copy link
Member

6543 commented Jan 12, 2020

thought it would change more ... 😅 - code change looks good and local test worked :)
... (because of version update)

@6543
6543 approved these changes Jan 12, 2020
@GiteaBot GiteaBot added lgtm/need 1 and removed lgtm/need 2 labels Jan 12, 2020
@silverwind silverwind changed the title move swagger-ui to webpack/npm move swagger-ui to webpack/npm and update it to 3.24.3 Jan 12, 2020
@sapk

This comment has been minimized.

Copy link
Member

sapk commented Jan 12, 2020

@silverwind doesn't this mean that the swagger.js will be build each time ?
It feel better to keep this outside from the gitea js config.

For info, by updating swagger-ui this will fix few security issues (but maybe not need a backport since only xss that we shouldn't be affected)

webpack.config.js Outdated Show resolved Hide resolved
@silverwind

This comment has been minimized.

Copy link
Member Author

silverwind commented Jan 12, 2020

doesn't this mean that the swagger.js will be build each time ?

It will only build once per change in the web_src directory, or when node_modules change.

Not 100% sure if make is smart enough to track changes in node_modules, so it might be better to track package-lock.json instead.

@silverwind

This comment has been minimized.

Copy link
Member Author

silverwind commented Jan 12, 2020

It feel better to keep this outside from the gitea js config.

I think it rightly belongs in the JS parts and swagger-ui also recommends the bundling approach.

webpack.config.js Show resolved Hide resolved
@sapk
sapk approved these changes Jan 12, 2020
Copy link
Member

sapk left a comment

I have doubt if this should be tightly configured to gitea js build but this is an improvement and it could be improved again later if it cause problem.

@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels Jan 12, 2020
Copy link
Member

techknowlogick left a comment

Thanks for PR, I’m on mobile and am having difficulty adding a comment so I am blocking this until I can get to a desktop to add review.

@silverwind

This comment has been minimized.

Copy link
Member Author

silverwind commented Jan 12, 2020

Investigating removal of source map for swagger.js as it's rather pointless for something that is 99.99% third party code and it will speed up the build.

@silverwind

This comment has been minimized.

Copy link
Member Author

silverwind commented Jan 12, 2020

@sapk I switched to swagger-ui-dist, let me know what you think. Now, it just copies the files to public during the webpack build. That way, build will be faster with the downside that we can no longer control the polyfills for the swagger-ui code.

@silverwind

This comment has been minimized.

Copy link
Member Author

silverwind commented Jan 12, 2020

Let's wait on @sapk to approve the latest change to swagger-ui-dist before merging. I did it based on request from #9715 (comment). Personally, I have a slight preference for the previous swagger-ui variant because it allows us to control the polyfills and it seems like the cleaner approach overall.

@silverwind

This comment has been minimized.

Copy link
Member Author

silverwind commented Jan 12, 2020

Decided to go with swagger-ui approach. It just seems cleaner that way. The webpack build completes in 6 seconds on my machine with swagger-ui included in the build, which seems plenty fast to me.

Good to go from my side.

@sapk

This comment has been minimized.

Copy link
Member

sapk commented Jan 12, 2020

@silverwind It is more the fact that it is in the same build process than gitea js that bother me the most compared to using directly release files but it feel more a personal intuition and not backed by real experience. So I am totally ok with merging this and see if it become a problem (which will be limited since only for builder) or if I was wrong to think so. Otherwise this PR is totally LGTM.

@silverwind

This comment has been minimized.

Copy link
Member Author

silverwind commented Jan 12, 2020

Okay. When it comes to JS modules, I don't expect release files to be present and many modules do not even provide them because it is generally expected that the user takes care of their bundling/polyfilling needs. swagger-ui actually goes the extra mile to provide those files separately, which is nice, but unnecessary given that we already bundle.

@sapk

This comment has been minimized.

Copy link
Member

sapk commented Jan 12, 2020

By release file, I mean like previously, totally outside npm/node_modules but don't bother with that.

@lafriks lafriks added this to the 1.12.0 milestone Jan 12, 2020
@lafriks lafriks added the kind/build label Jan 12, 2020
@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Jan 12, 2020

I would also prefer that we build dependencies ourselves as most libraries in npm are provided with idea to be built on end product

@silverwind silverwind force-pushed the silverwind:swagger-ui-npm branch from 0d57e5a to 14889ec Jan 12, 2020
@silverwind

This comment has been minimized.

Copy link
Member Author

silverwind commented Jan 12, 2020

Squashed and pushed a few minor cosmetic tweaks.

@silverwind

This comment has been minimized.

Copy link
Member Author

silverwind commented Jan 12, 2020

CI failed with unrelated error, please restart.

level=error msg="Running error: ctrlflow: failed prerequisites: inspect@code.gitea.io/gitea/modules/markup/markdown [code.gitea.io/gitea/modules/markup.test]"
Created a second webpack output file for swagger-ui which is loaded on
the /api/swagger route. One notable difference is the absence of the
swagger favicon that was previously used which is now the gitea icon. I
see no easy way to restore that favicon, so I decided to not keep it.
@silverwind silverwind force-pushed the silverwind:swagger-ui-npm branch from 14889ec to fd40e58 Jan 14, 2020
@silverwind

This comment has been minimized.

Copy link
Member Author

silverwind commented Jan 14, 2020

Rebased. @techknowlogick want to lift your block?

@techknowlogick techknowlogick dismissed their stale review Jan 14, 2020

dismiss block

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 14, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9714      +/-   ##
==========================================
- Coverage   42.32%   42.17%   -0.16%     
==========================================
  Files         600      592       -8     
  Lines       78357    78155     -202     
==========================================
- Hits        33161    32958     -203     
- Misses      41140    41150      +10     
+ Partials     4056     4047       -9
Impacted Files Coverage Δ
models/repo_branch.go 50.43% <0%> (-49.57%) ⬇️
models/context.go 8.33% <0%> (-37.5%) ⬇️
routers/api/v1/repo/issue.go 35.09% <0%> (-18.32%) ⬇️
services/repository/repository.go 47.91% <0%> (-8.34%) ⬇️
models/repo_generate.go 3.8% <0%> (-6.62%) ⬇️
models/access.go 66.32% <0%> (-5.53%) ⬇️
services/pull/temp_repo.go 31.62% <0%> (-2.57%) ⬇️
models/action.go 46.87% <0%> (-2.53%) ⬇️
models/repo_list.go 74.42% <0%> (-2.39%) ⬇️
routers/api/v1/org/org.go 71.17% <0%> (-1.34%) ⬇️
... and 47 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 44de66b...fd40e58. Read the comment docs.

@techknowlogick techknowlogick merged commit f00961a into go-gitea:master Jan 14, 2020
2 checks passed
2 checks passed
approvals/lgtm this commit looks good
continuous-integration/drone/pr Build is passing
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
You can’t perform that action at this time.