Bundle all vendor libs together .... #30088
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
partly discussed in #17169 |
PVince81
reviewed
Jan 10, 2018
| @@ -0,0 +1,42 @@ | |||
| var $ = require('jquery'); | |||
There was a problem hiding this comment.
Ok so it seems this file is still some kind of weird workaround to make libraries available at load time.
I expect that this one will disappear once we start using require() in all our JS code that actually requires these libs. And also once we use webpack for other things...
@felixheidecke any thoughts on this ?
There was a problem hiding this comment.
I'll have a look to make this work with ProvidePlugin ...
There was a problem hiding this comment.
@PVince81 are we moving to ES6 already? Would be nice. :-D
PVince81
reviewed
Jan 10, 2018
| @@ -1,6 +1,6 @@ | |||
| <?php /** @var $l \OCP\IL10N */ ?> | |||
| <?php | |||
| vendor_script('jsTimezoneDetect/jstz'); | |||
|
Nice! |
DeepDiver1975
force-pushed
the
use-webpack-for-core-vendor
branch
3 times, most recently
from
0521d9f
to
856ab46
Compare
Codecov Report
@@ Coverage Diff @@
## master #30088 +/- ##
============================================
+ Coverage 63.95% 63.95% +<.01%
- Complexity 18592 18593 +1
============================================
Files 1172 1172
Lines 69820 69811 -9
Branches 1267 1267
============================================
- Hits 44654 44650 -4
+ Misses 24797 24792 -5
Partials 369 369
Continue to review full report at Codecov.
|
|
The UI tests fail that use the sharing panel. Locally for me I do not get any list of files in the webUI, no menus,... This is after doing |
make clean & make should pull in everything what is needed .... |
|
After
I tried stuff like: but does not help. Is there some extra magic needed here? |
|
hmmm .... npx is part of node 8. We need a different approach here ... Workaround: cd build
webpack |
|
maybe just add "build/node_modules/.bin" to the PATH inside the Makefile before running command? |
|
or if this is about running webpack from CLI, just add it here: https://github.com/owncloud/core/blob/master/Makefile#L42 |
yarn already sets the PATH ... but there is a bug when calling yarn with --cwd ... |
|
I guess it's better to add the webpack call to the make file because webpack needs to be executed as soon as package.json, webpack.config.js or entry.js changes - we can only handle this properly in the Makefile afaik. I'd like to get some help with this ;-) Furthermore we should take a closer look on the libs - afaik Promise is only needed for IE - right? We might want to have two different bundle.js - one for ie and one for other browsers. no idea what is the best solution - some webpack practicioneer support would be great ;-) |
|
I've added webpack to the Makefile. Note that if you edit package.json or the webpack config and run |
DeepDiver1975
commented
Jan 12, 2018
Makefile
Outdated
| cd $(NODE_PREFIX) && $(YARN) install | ||
| touch $@ | ||
|
|
||
| # alias for core deps | ||
| $(core_vendor): $(nodejs_deps) | ||
| $(core_vendor): $(nodejs_deps) $(NODE_PREFIX)/webpack.config.js |
There was a problem hiding this comment.
entry.js should also be a criteria to rerun this step
Makefile
Outdated
| cd $(NODE_PREFIX) && $(YARN) install | ||
| touch $@ | ||
|
|
||
| # alias for core deps | ||
| $(core_vendor): $(nodejs_deps) | ||
| $(core_vendor): $(nodejs_deps) $(NODE_PREFIX)/webpack.config.js | ||
| mkdir -p $(core_vendor) |
There was a problem hiding this comment.
add a rm -rf to get a clean vendor folder?
build/webpack.config.js
Outdated
| @@ -20,7 +20,6 @@ module.exports = { | |||
| // Promise: 'es6-promise' | |||
| }), | |||
| new CopyWebpackPlugin([ | |||
| { from: 'browser-update/browser-update.js', to: 'browser-update/browser-update.js' }, | |||
|
@DeepDiver1975 comments adressed + some additional changes to make |
|
The |
|
Javascript tests fail: https://drone.owncloud.com/owncloud/core/2452/16 However running It turns out test-javascript.sh is not using the makefile. We need to change that. Also yarn-install.sh should not exist, we should use make, that's what it's for: installing deps. |
DeepDiver1975
force-pushed
the
use-webpack-for-core-vendor
branch
2 times, most recently
from
574e22f
to
ef5e40c
Compare
|
Damn, I thought this was already merged. Blocking #29815 |
DeepDiver1975
force-pushed
the
use-webpack-for-core-vendor
branch
from
3010d3b
to
610cdc3
Compare
|
While investigating #32117 (comment) I tried moving "oc-backbone-webdav" into the template.php, directly after "oc-backbone". But then when I reloaded the page the order was wrong and different. This made me realize that the order in which Why is this information relevant to this PR here ? I believe that this PR was written assuming that the order of So the plan is to reverify the webpack wrapper script and reorder the imports to match what we see in the
|
|
here we go, the reference list: |
Should fix select2 issues
|
I've now reordered the vendor libs in "entry.js" to match the order of vendor libs from the list above. Unfortunately the autocomplete still doesn't work. I noticed the following:
Needs further research and deeper look at the order. Maybe we first wait for the jquery.ui update before continuing ? #18739 |
|
Seems we updated jquery.ui by mistake by using the I'm now trying to enforce using the same versions like we had on master. Updating jquery and jquery.ui are separate tasks on their own. |
|
(will push later, still testing locally...) |
|
ok, apparrently |
added 2 commits
August 3, 2018 19:07
We only need to load the custom one
|
Still same problem... One clue I see is that something looks wrong with the jquery ui CSS. With a few hacks I'm able to make the autocomplete field appear, and here we see that the selection items look completely different than on master: |
|
I think I'm onto something: on this PR the vendor libs are loaded after the jquery-ui-fixes.css is loaded. The challenge we have here is that the vendor.js bundle contains both JS and CSS. Maybe we should separate the JS and CSS and have the vendor bundle contain only JS, and find another way to load CSS. Maybe have them concatenated into a vendor CSS file. @felixheidecke thoughts ? |
|
I've now managed to make the jquery-ui* CSS load in the same order as above by simply copying the CSS over with Sadly this doesn't solve the autocomplete problem... Next up to try: have jquery.ui 0.10.0 in here instead of 0.10.5... |
This ensures that the loading order is the same as before on master.
|
on master the version in yarn.lock is: |
|
I tried changing the version from
😢 |
|
AHA, a new insight: I went to master and updated jquery.ui from 1.10.0 to 1.10.4 in the bower dependencies. After that update the autocomplete stopped worked with the same symptoms (z-index problem). This confirms that the main problem now is the forced update to 1.10.4. I didn't find a nice way to make yarn load jquery-ui 1.10.0 without bower, due to #30088 (comment). Maybe we'll need to wait for #18739 to have a clean jquery.ui update. The other alternative is trying to adjust our jquery-ui-fixes.css to properly override the styles of 1.10.4... The worse part here is mostly that we had this jquery-ui-fixes file in the first place... |
|
At this point I have the feeling that there is no point trying to hack this PR further before jquery-ui is updated and all related hacks as part of #18739. |
Merged
|
we now have jquery ui update and jquery 2 on master, so could carry on with this PR if we still think it's important |
|
deferred for now. also Phoenix brings its own UI components now, so likely obsolete |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Use webpack to bundle vendor libs into one bundle.js
Related Issue
@PVince81 I don't find the related issue anymore.
Motivation and Context
How Has This Been Tested?
Types of changes
Checklist: