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 jquery and jquery-migrate to npm/webpack #9813

Merged
merged 1 commit into from
Jan 21, 2020

Conversation

silverwind
Copy link
Member

Currently, this needs to be its own chunk because fomantic depends on jQuery being present. The next step is to move fomantic to webpack too after which we can combine the index,fomantic and jquery files into one.

jquery-migrate is still neccessary because our ancient version of Dropzone seems to break without it. I imagine it can be removed after a Dropzone upgrade.

@codecov-io
Copy link

codecov-io commented Jan 17, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@0e8b27a). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #9813   +/-   ##
=========================================
  Coverage          ?   42.31%           
=========================================
  Files             ?      605           
  Lines             ?    79313           
  Branches          ?        0           
=========================================
  Hits              ?    33561           
  Misses            ?    41610           
  Partials          ?     4142

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 0e8b27a...963dfa8. Read the comment docs.

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

Regarding future interaction between jQuery and fomantic: Problem is jQuery does not set globals when loaded via webpack but they must be present for fomantic to load correctly. I imagine some experimenting with one of these might get it to work, but it will be a bit tricky:

webpack.config.js Outdated Show resolved Hide resolved
@lafriks lafriks added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Jan 17, 2020
@lafriks lafriks added this to the 1.12.0 milestone Jan 17, 2020
@silverwind
Copy link
Member Author

silverwind commented Jan 17, 2020

I think I might opt to use copy-webpack-plugin for jQuery to deal with the future fomantic situation, so this should be considered WIP for now.

@lafriks
Copy link
Member

lafriks commented Jan 17, 2020

I think better currently leave as it is now, and when moving fomantic than we can think about copy-webpack-plugin

@silverwind
Copy link
Member Author

Agree, that can be done later, so this should be good to merge for now.

@silverwind
Copy link
Member Author

Rebased and included a tweak to webpack which eliminates asset size warnings currently present on master.

This is good to go from my side. Any reviews?

@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 Jan 20, 2020
@lunny
Copy link
Member

lunny commented Jan 21, 2020

Please resolve the conflicts.

@silverwind
Copy link
Member Author

Rebased and squashed.

Currently, this needs to be its own chunk because fomantic depends
on jQuery being present. The next step is to move fomantic to webpack
too after which we can combine the index,fomantic and jquery files into
one.

jquery-migrate is still neccessary because our ancient version of Dropzone
seems to break without it. I imagine it can be removed after a Dropzone
upgrade.
@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 Jan 21, 2020
@lafriks lafriks merged commit 2982afe into go-gitea:master Jan 21, 2020
@silverwind silverwind deleted the jquery-npm branch January 21, 2020 19:30
@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. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants