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

[6.0.0alpha] Actions infinite loop regression #2902

Closed
PVince81 opened this issue Jul 28, 2022 · 16 comments · Fixed by #2911
Closed

[6.0.0alpha] Actions infinite loop regression #2902

PVince81 opened this issue Jul 28, 2022 · 16 comments · Fixed by #2911
Labels
bug Something isn't working regression Regression of a previous working feature
Milestone

Comments

@PVince81
Copy link
Contributor

See nextcloud/server#33387 (comment)

@Pytal can you investigate ?

@PVince81 PVince81 added bug Something isn't working regression Regression of a previous working feature labels Jul 28, 2022
@PVince81 PVince81 added this to the 6.0.0 milestone Jul 28, 2022
@PVince81
Copy link
Contributor Author

I suggest to first revert all changes in AppSidebar to see if the problem persists, in case it's only a side effect.

@Pytal
Copy link
Contributor

Pytal commented Jul 28, 2022

Findings as follows

The infinite loop is not triggered in AppSidebar but rather in Actions

The recent changes which touch related code are these fixes #2878 and #2532

With the isValidSingleAction fix the markup https://github.com/nextcloud/nextcloud-vue/blob/79f3c499f14c8d188c93da2258bb690977bfa8f3/src/components/Actions/Actions.vue#L135-L159 which depends on firstAction renders as expected but because children change at some point during the render lifecycle the watcher https://github.com/nextcloud/nextcloud-vue/blob/79f3c499f14c8d188c93da2258bb690977bfa8f3/src/components/Actions/Actions.vue#L477-L488 mutates firstAction while rendering leading to the "infinite update loop in a component render function" error, commenting out the assignment removes the error

Wrapping it in $nextTick doesn't seem to work so any suggestions here are welcome @PVince81

@PVince81
Copy link
Contributor Author

@vinicius73 @raimund-schluessler @marcoambrosini please help solve the infinite loop in the Actions component. I picked you as you are connected to the matching PRs.

@raimund-schluessler
Copy link
Contributor

Findings as follows

The infinite loop is not triggered in AppSidebar but rather in Actions

The recent changes which touch related code are these fixes #2878 and #2532

With the isValidSingleAction fix the markup https://github.com/nextcloud/nextcloud-vue/blob/79f3c499f14c8d188c93da2258bb690977bfa8f3/src/components/Actions/Actions.vue#L135-L159 which depends on firstAction renders as expected but because children change at some point during the render lifecycle the watcher https://github.com/nextcloud/nextcloud-vue/blob/79f3c499f14c8d188c93da2258bb690977bfa8f3/src/components/Actions/Actions.vue#L477-L488 mutates firstAction while rendering leading to the "infinite update loop in a component render function" error, commenting out the assignment removes the error

Wrapping it in $nextTick doesn't seem to work so any suggestions here are welcome @PVince81

Does reverting #2878 fix the issue? Could also be related to the migration to floating-vue, although given the investigation above, I find it rather not likely. I can only have a look on Tuesday, sorry.

@raimund-schluessler
Copy link
Contributor

Honestly, I find the Actions component a bit messy and wanted to rewrite it / clean it up. But I planned to add some tests for it in #2881 first, just haven't had the time to do it yet, though.

@marcoambrosini
Copy link
Contributor

Does reverting #2878 fix the issue?

I've done some testing with and it doesn't seem so :/

I've also tried to link nextcloud/vue to server but it fails to build due to a problem with material design icons

ERROR in ./node_modules/@nextcloud/vue/node_modules/vue-material-design-icons/ToggleSwitchOff.vue 1:0
Module parse failed: Unexpected token (1:0)
You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
> <template>
|   <span v-bind="$attrs"
|         :aria-hidden="!title"
 @ ./node_modules/@nextcloud/vue/dist/Components/CheckboxRadioSwitch.js 2:79915-79967
 @ ./node_modules/babel-loader/lib/index.js!./node_modules/vue-loader/lib/index.js??vue-loader-options!./apps/settings/src/components/AdminTwoFactor.vue?vue&type=script&lang=js& 69:0-85 79:25-44
 @ ./apps/settings/src/components/AdminTwoFactor.vue?vue&type=script&lang=js& 1:0-186 1:202-205 1:207-390 1:207-390
 @ ./apps/settings/src/components/AdminTwoFactor.vue 2:0-66 3:0-61 3:0-61 10:2-8
 @ ./apps/settings/src/main-admin-security.js 26:0-61 36:22-36

@raimund-schluessler
Copy link
Contributor

Does reverting #2878 fix the issue?

I've done some testing with and it doesn't seem so :/

I've also tried to link nextcloud/vue to server but it fails to build due to a problem with material design icons


ERROR in ./node_modules/@nextcloud/vue/node_modules/vue-material-design-icons/ToggleSwitchOff.vue 1:0

Module parse failed: Unexpected token (1:0)

You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders

> <template>

|   <span v-bind="$attrs"

|         :aria-hidden="!title"

 @ ./node_modules/@nextcloud/vue/dist/Components/CheckboxRadioSwitch.js 2:79915-79967

 @ ./node_modules/babel-loader/lib/index.js!./node_modules/vue-loader/lib/index.js??vue-loader-options!./apps/settings/src/components/AdminTwoFactor.vue?vue&type=script&lang=js& 69:0-85 79:25-44

 @ ./apps/settings/src/components/AdminTwoFactor.vue?vue&type=script&lang=js& 1:0-186 1:202-205 1:207-390 1:207-390

 @ ./apps/settings/src/components/AdminTwoFactor.vue 2:0-66 3:0-61 3:0-61 10:2-8

 @ ./apps/settings/src/main-admin-security.js 26:0-61 36:22-36

The problem also occurs in the Tasks app dashboard widget: nextcloud/tasks#2056 Maybe that's easier to debug.

@Pytal Pytal changed the title [6.0.0alpha] AppSidebar infinite loop regression [6.0.0alpha] Actions infinite loop regression Jul 29, 2022
@Pytal
Copy link
Contributor

Pytal commented Jul 29, 2022

I've also tried to link nextcloud/vue to server but it fails to build due to a problem with material design icons

Removing these lines https://github.com/nextcloud/server/blob/952acd4d276b3190d23e0597c5e01b1dfc4d72bc/webpack.common.js#L82-L85 fixes the build error @marcoambrosini @raimund-schluessler

After some more tinkering it's seems that floating-vue #2600 may be the actual source of the issue

@PVince81

This comment was marked as outdated.

@PVince81

This comment was marked as outdated.

@PVince81
Copy link
Contributor Author

PVince81 commented Aug 4, 2022

I retracted my comments. It seems my setup is having trouble with npm link and even with v5 the problem happens.

@raimund-schluessler
Copy link
Contributor

raimund-schluessler commented Aug 4, 2022

I retracted my comments. It seems my setup is having trouble with npm link and even with v5 the problem happens.

I see problems with npm link as well, when having an app that uses [email protected] and linking it with nextcloud/vue master. Bumping vue to 2.7.8 in the nextcloud/vue repo fixes this, see #2929.

But I also think that the problem is not really due to floating-vue, or at least is only triggered by it and in reality lies in our Actions component implementation. I currently rewrite it in #2911. For me, it fixes the infinite loop problem, but there are still some things missing in the PR.

@PVince81
Copy link
Contributor Author

PVince81 commented Aug 4, 2022

I made a local branch with both #2929 and reverting floating-vue: now the share panel opens in the files app but I still get the infinite loop warning from Vue. The problem has good "hybrid" or they are two completely separate problems.

@raimund-schluessler
Copy link
Contributor

@PVince81 @Pytal Could you please check whether #2911 fixes the issue for you? It does for me, and it would be great to know if it works for you as well 🙂

@Pytal
Copy link
Contributor

Pytal commented Aug 8, 2022

Tested it and it's fixed @raimund-schluessler!

@raimund-schluessler
Copy link
Contributor

Tested it and it's fixed @raimund-schluessler!

Great, then let's merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working regression Regression of a previous working feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants