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.

Sign up for GitHub

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

Jump to bottom

fix(files): do not load legacy files app js #40065

Merged
merged 3 commits into from
Sep 27, 2023
Merged

fix(files): do not load legacy files app js #40065

merged 3 commits into from
Sep 27, 2023

Conversation

max-nextcloud
Copy link
Contributor

OCP.Files.Navigation is undefined.

cypress-screenshot

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Tests are fixed by this
  • Screenshot of test failurs
  • Documentation is not required
  • Backports are not required

@skjnldsv
Copy link
Member

skjnldsv commented Aug 27, 2023 •
edited
Loading

Where is legacy files still loaded?
Those scripts should not even be present in the page anymore :)

@max-nextcloud max-nextcloud force-pushed the bugfix/files/navigation-api branch from 58ab539 to e43a2ab Compare August 27, 2023 10:19
@max-nextcloud
Copy link
Contributor Author

uh... interesting - i will investigate.

@max-nextcloud
Copy link
Contributor Author

Loaded directly from the html inside the cypress tests. I will see if it also loads without cypress...

grafik

@max-nextcloud
Copy link
Contributor Author

It's served with the initial template when loading apps/files:
grafik

@skjnldsv
Copy link
Member

skjnldsv commented Aug 27, 2023 •
edited
Loading

Ah, found
This ain't supposed to still be there

server/apps/files/js/app.js

Line 403 in 3846ec2

OCA.Files.App.initialize();

EDIT: but it might still be used in public mode 🤔

@skjnldsv
Copy link
Member

skjnldsv commented Aug 27, 2023 •
edited
Loading

It's a multi step, but requires a bit more:

  • Do not load the files merged js
  • Remove systemtagsfilelist js
  • Fix Sidebar to not rely on OCA.Files.App.fileList.filesClient

    server/apps/files/src/services/FileInfo.js

    Lines 36 to 39 in be41dc4

    // TODO: create new parser or use cdav-lib when available
    const file = OCA.Files.App.fileList.filesClient._client.parseMultiStatus(response.data)
    // TODO: create new parser or use cdav-lib when available
    const fileInfo = OCA.Files.App.fileList.filesClient._parseFileInfo(file[0])

Then it should be good, public pages seems to still work, I gues they load their own minimal stuff

@max-nextcloud

This comment was marked as outdated.

Sign in to view
@skjnldsv
Copy link
Member

if we don't include the file, dropping the initialize will not matter.
I think we can keep those files here until #39914 is complete and we do a cleanup pass

@max-nextcloud max-nextcloud force-pushed the bugfix/files/navigation-api branch from e43a2ab to ae66c40 Compare August 27, 2023 12:31
@max-nextcloud
Copy link
Contributor Author

I think I got it... or at least some intermediate state.

Circles is missing the filesList though:
grafik

@max-nextcloud max-nextcloud mentioned this pull request Aug 27, 2023
Merged
10 tasks
@max-nextcloud max-nextcloud changed the title fix(files): use window._nc_navigation in legacy file fix(files): do not load legacy files app js Aug 27, 2023
@skjnldsv
Copy link
Member

Circles is missing the filesList though:

Circles is not upgraded for 28 though 🤔
I think that's a different topic anyway

@max-nextcloud max-nextcloud requested a review from skjnldsv August 27, 2023 14:54
max-nextcloud
max-nextcloud commented Aug 27, 2023
View reviewed changes
apps/files/lib/Controller/ViewController.php Outdated
@@ -218,7 +218,7 @@ public function index($dir = '', $view = '', $fileid = null, $fileNotFound = fal

// Load the files we need
\OCP\Util::addStyle('files', 'merged');
\OCP\Util::addScript('files', 'merged-index', 'files');
\OCP\Util::addScript('files', 'fileactions');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without the fileactions import files won't open anymore in the viewer. So I guess we still need it.

@skjnldsv - what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried latest Viewer?
We don't use the old fileactions ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes... latest viewer, disabled contacts app which was causing errors and early return from circles app so it does not interfere. Still .md .pdf and .jpgs have no view action and clicking files triggers a download. Did not try other file types.

@max-nextcloud max-nextcloud force-pushed the bugfix/files/navigation-api branch from bf8d915 to e17a725 Compare August 27, 2023 14:56
max-nextcloud added a commit to nextcloud/text that referenced this pull request Aug 27, 2023
@max-nextcloud
tmp(cypress): ignore files cannot read views error
51b66dc 
See also nextcloud/server#40065
max-nextcloud added a commit to nextcloud/text that referenced this pull request Aug 27, 2023
@max-nextcloud
tmp(cypress): ignore files cannot read views error
dc29ff9 
See also nextcloud/server#40065

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud max-nextcloud mentioned this pull request Aug 27, 2023
Closed
4 tasks
@skjnldsv
Copy link
Member

Is it still relevant?

@max-nextcloud max-nextcloud force-pushed the bugfix/files/navigation-api branch from e17a725 to c7c331b Compare September 25, 2023 17:24
@max-nextcloud
Copy link
Contributor Author

Is it still relevant?

I think so. Cleaned it up and the main thing now is not to addScript('files', 'merged-index', 'files');.

@skjnldsv skjnldsv requested review from a team, susnux and szaimen and removed request for a team September 26, 2023 11:42
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 26, 2023
@skjnldsv skjnldsv added this to the Nextcloud 28 milestone Sep 26, 2023
@max-nextcloud
fix(files): do not include scripts for merge-index
7ba6409 
Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
fix(files): use OC.Files.getClient()
27bcef7 
`OCA.Files.App.fileList` is not initiated anymore.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud max-nextcloud force-pushed the bugfix/files/navigation-api branch from c7c331b to 27bcef7 Compare September 26, 2023 19:37
@max-nextcloud
Copy link
Contributor Author

/compile

@nextcloud-command
chore(assets): Recompile assets
360ae11 
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@skjnldsv skjnldsv disabled auto-merge September 27, 2023 06:33
@skjnldsv skjnldsv merged commit 5dcefad into master Sep 27, 2023
34 of 36 checks passed
@skjnldsv skjnldsv deleted the bugfix/files/navigation-api branch September 27, 2023 06:33
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Sep 27, 2023
@skjnldsv skjnldsv mentioned this pull request Oct 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skjnldsv skjnldsv skjnldsv approved these changes

@susnux susnux susnux approved these changes

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@szaimen szaimen Awaiting requested review from szaimen szaimen was automatically assigned from nextcloud/server-frontend

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: files
Projects
Files to vue
Status: Done
Milestone
Nextcloud 28
Development

Successfully merging this pull request may close these issues.
4 participants
@max-nextcloud @skjnldsv @susnux @nextcloud-command