Migrate Update notifications to Vue.js #7933

nickvergessen · 2018-01-18T11:28:35Z

No description provided.

nickvergessen · 2018-01-18T11:29:16Z

apps/updatenotification/templates/admin.php

+ * This file is licensed under the Affero General Public License version 3 or
+ * later. See the COPYING file.
+ */
+if (\OC::$server->getConfig()->getSystemValue('debug', false)) {


Should we add a helper method for this block? or not worth it?

Makes sense from my POV. I've seen this more and more in apps to get some kind of debug setup for JS code.

nickvergessen · 2018-01-18T11:30:02Z

apps/updatenotification/js/components/root.js

+		},
+
+		updated: function () {
+			OC.Settings.setupGroupsSelect(this._$notifyGroups);


Saving is currently not working, need to look into migrating this to:
https://vuejs.org/v2/examples/select2.html

Or better without jQuery: https://github.com/vuejs/awesome-vue#select

nickvergessen · 2018-01-18T11:31:24Z

apps/updatenotification/js/admin.js

+		 */
+		initialise: function() {
+			var data = JSON.parse($('#updatenotification').attr('data-json'));
+			this.vm = new Vue(OCA.UpdateNotification.Components.Root);


@nextcloud/javascript How can I make sure this uses the version we load from the updatenotification/js/vue.js instead of the version from e.g. mail?

Or how do we handle this, when there are multiple different versions on one instance because of multiple apps?

Other apps should only load a library like vue when they are in control over the current route. I don't see a reason why the vue version from e.g. mail would be loaded on the settings page for updates.

Maybe it might make sense to have a common vue shipped with the server so that at least all settings / shipped apps can use the same without having to much library code being distributed across them.

Other apps should only load a library like vue when they are in control over the current route. I don't see a reason why the vue version from e.g. mail would be loaded on the settings page for updates.

Works, until you have an app like notifications, which is loaded on each logged in page that shows the header.
Also in this app, it's loaded in the admin settings. A second app could register for the same section and it would be loaded twice again.

That is why I wondered if we should ship a version in core instead, ref #2692 (comment)

Shipping a version in core has the disadvantage, that it get's hard to update. Is there no way to load your stuff totally isolated in it's own namespace and not polluting the global namespace?

Shipping a version in core has the disadvantage, that it get's hard to update. Is there no way to load your stuff totally isolated in it's own namespace and not polluting the global namespace?

Sure. Just use a bundling tool like webpack and you'll get a single .js file that contains all your app+vendor scripts without interfering with the global libraries. This way, we ship our own, up-to-date version of jQuery, Backbone and Underscore in mail.

Further, this a more general question. The vue library with compiler is quite big. Using webpack et. al. also removes the need for the compiler. These two parts speed up the code.

https://github.com/nextcloud/twofactor_u2f/pull/74/files could be a reference PR where I also converted an app using simple global scripts to webpack.

Basically you need the following

npm config to define and download your dependencies

webpack config for bundling (with one or more entrypoints)

migrate scripts to modules

Since this will introduce another build step, do you plan to commit the build artifacts?

FYI: If not, people will have to run the build script themselves to use the app. If you do, then it will cause problems with merge conflicts, rebases and git bisects where the built bundle is outdated.

How can we keep the easy development setup then? It would add an additional step to build the js code, which we always tried to avoid for the server part in the past.

How can we keep the easy development setup then? It would add an additional step to build the js code, which we always tried to avoid for the server part in the past.

Correct. And we should discuss this in a separate ticket 😉

codecov · 2018-01-18T11:56:21Z

Codecov Report

Merging #7933 into master will increase coverage by 0.09%.
The diff coverage is 57.89%.

@@             Coverage Diff              @@
##             master    #7933      +/-   ##
============================================
+ Coverage     51.75%   51.85%   +0.09%     
- Complexity    25370    25450      +80     
============================================
  Files          1601     1602       +1     
  Lines         95012    95256     +244     
  Branches       1377     1379       +2     
============================================
+ Hits          49178    49394     +216     
- Misses        45834    45862      +28

Impacted Files	Coverage Δ	Complexity Δ
apps/updatenotification/templates/admin.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...pps/updatenotification/lib/AppInfo/Application.php	`7.4% <0%> (ø)`	`10 <0> (ø)`	⬇️
apps/updatenotification/lib/Settings/Admin.php	`73.58% <68.75%> (-3.92%)`	`11 <3> (+3)`
settings/Middleware/SubadminMiddleware.php	`46.42% <0%> (-46.43%)`	`12% <0%> (+6%)`
...ddleware/Security/Exceptions/NotAdminException.php	`66.66% <0%> (-33.34%)`	`2% <0%> (+1%)`
...amework/Middleware/Security/SecurityMiddleware.php	`58.18% <0%> (-19.87%)`	`52% <0%> (+26%)`
...e/AppFramework/DependencyInjection/DIContainer.php	`68.42% <0%> (-14.31%)`	`85% <0%> (+31%)`
apps/user_ldap/lib/Connection.php	`54.74% <0%> (-3.33%)`	`122% <0%> (+3%)`
apps/files_trashbin/lib/Expiration.php	`90.32% <0%> (-1.62%)`	`29% <0%> (ø)`
lib/private/L10N/L10N.php	`85.96% <0%> (-1.31%)`	`5% <0%> (+1%)`
... and 17 more

ChristophWurst · 2018-01-19T08:33:43Z

apps/updatenotification/js/components/root.js

+			 * Creates a new authentication token and loads the updater URL
+			 */
+			clickUpdaterButton: function() {
+				$.ajax({


FYI I'd suggest to use js promises that actually allow you to nicely chain async method calls like these here and avoid the callback hell.

Then it would be something like

$.ajax(…) .then(function(data) { … }) .then(function(data) { … }) .catch(function(e) { … handle error … })

I also did this in the u2f app where lots of async calls are necessary
https://github.com/nextcloud/twofactor_u2f/blob/796ea5495a3c7bc86b4e0956df98a20877a7845e/js/settingsview.js#L205-L223

Thanks, will have a look. At the moment this is just a copy paste of the original

nickvergessen · 2018-01-31T09:59:53Z

@skjnldsv

Or better without jQuery: https://github.com/vuejs/awesome-vue#select

I used that now. But the styling of the options and the dropdown needs some adjustments due to css class colisions:

can you help with that?

skjnldsv · 2018-01-31T17:15:16Z

can you help with that?

nickvergessen · 2018-02-22T08:10:44Z

Maybe we can merge this with the CSS problem, so I can continue with merging the "Can I update?" app before we start with the Nextcloud 14 features.

Signed-off-by: Joas Schilling <[email protected]>

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

skjnldsv · 2018-02-22T16:01:05Z

@nickvergessen

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Signed-off-by: Joas Schilling <[email protected]>

nickvergessen · 2018-02-23T08:15:36Z

Works nicely, thanks for the help!

Fixed the unit tests, so now really ready for review

skjnldsv

Okay from me

skjnldsv · 2018-02-25T10:40:58Z

Test restarted, failure unrelated :)

Signed-off-by: Morris Jobke <[email protected]>

MorrisJobke · 2018-02-26T16:48:11Z

apps/updatenotification/Makefile

@@ -1,4 +1,4 @@
-app_name=notifications
+app_name=updatenotification


@nickvergessen I fixed this 😉

🙈 thanks

nickvergessen added enhancement 2. developing Work in progress feature: install and update feature: activity and notification labels Jan 18, 2018

nickvergessen added this to the Nextcloud 14 milestone Jan 18, 2018

nickvergessen requested a review from MorrisJobke January 18, 2018 11:28

nickvergessen commented Jan 18, 2018

View reviewed changes

ChristophWurst reviewed Jan 19, 2018

View reviewed changes

nickvergessen mentioned this pull request Jan 19, 2018

Webpack for server apps #7950

Closed

nickvergessen force-pushed the update-notification-vuejs branch from 0d06988 to 6dfcaf9 Compare January 31, 2018 09:54

skjnldsv self-assigned this Jan 31, 2018

nickvergessen force-pushed the update-notification-vuejs branch from 6dfcaf9 to 18e0204 Compare February 1, 2018 10:09

nickvergessen force-pushed the update-notification-vuejs branch from 18e0204 to 88f890a Compare February 14, 2018 16:22

nickvergessen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 22, 2018

nickvergessen added 5 commits February 22, 2018 10:17

Rename legacy notification so it's less confusing

52f8d75

Signed-off-by: Joas Schilling <[email protected]>

Migrate the app to Vue.js

b12b391

Signed-off-by: Joas Schilling <[email protected]>

Move updatenotifications to webpack with .vue file

308c7db

Signed-off-by: Joas Schilling <[email protected]>

Use VueSelect instead of select2

59c007b

Signed-off-by: Joas Schilling <[email protected]>

Use mixin for translations

6340e78

Signed-off-by: Joas Schilling <[email protected]>

nickvergessen force-pushed the update-notification-vuejs branch from 88f890a to 6340e78 Compare February 22, 2018 09:22

Ignore the map file too

96e9173

Signed-off-by: Joas Schilling <[email protected]>

skjnldsv mentioned this pull request Feb 22, 2018

External storage design fixes and update #8496

Merged

3 tasks

Add vselect theming into server

5c3672a

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

skjnldsv and others added 2 commits February 22, 2018 17:03

fixup! Add vselect theming into server

47af421

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>

Fix unit tests

69c88ad

Signed-off-by: Joas Schilling <[email protected]>

skjnldsv approved these changes Feb 23, 2018

View reviewed changes

nickvergessen unassigned skjnldsv Feb 26, 2018

Fix appname

955e136

Signed-off-by: Morris Jobke <[email protected]>

MorrisJobke reviewed Feb 26, 2018

View reviewed changes

MorrisJobke approved these changes Feb 26, 2018

View reviewed changes

MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 26, 2018

nickvergessen merged commit abb0a08 into master Feb 26, 2018

nickvergessen deleted the update-notification-vuejs branch February 26, 2018 17:55

		@@ -1,4 +1,4 @@
		app_name=notifications
		app_name=updatenotification

Uh oh!

Migrate Update notifications to Vue.js #7933

Migrate Update notifications to Vue.js #7933

Uh oh!

Conversation

nickvergessen commented Jan 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickvergessen commented Jan 31, 2018

Uh oh!

skjnldsv commented Jan 31, 2018

Uh oh!

nickvergessen commented Feb 22, 2018

Uh oh!

skjnldsv commented Feb 22, 2018

Uh oh!

nickvergessen commented Feb 23, 2018

Uh oh!

skjnldsv left a comment

Choose a reason for hiding this comment

Uh oh!

skjnldsv commented Feb 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov bot commented Jan 18, 2018 •

edited

Loading