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

Allow scripts prioritization based on other apps #30015

Merged
merged 2 commits into from
Dec 2, 2021

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Dec 1, 2021

  1. This allow to set after which app some scripts should be loaded.
  2. This creates a bit more flexibility and control over script dependencies.
  3. This deprecates the current old addScript legacy behaviour

Example given:

Before we had to rely on luck, then use a watcher to catch any initialization that came after Viewer got updated.
Now, I can only check on viewer initialization and that's it.

Rules:

  1. Core always gets prioritized
  2. You can specify only ONE dependency
  3. If the dependency does NOT exists, it will fallback to the php process order (default current)

@skjnldsv skjnldsv added this to the Nextcloud 24 milestone Dec 1, 2021
@skjnldsv skjnldsv requested review from ChristophWurst and a team December 1, 2021 17:20
@skjnldsv skjnldsv self-assigned this Dec 1, 2021
@skjnldsv skjnldsv requested review from PVince81, Pytal, szaimen, icewind1991 and come-nc and removed request for a team December 1, 2021 17:20
@skjnldsv skjnldsv added the pending documentation This pull request needs an associated documentation update label Dec 1, 2021
lib/public/Util.php Show resolved Hide resolved
lib/public/Util.php Outdated Show resolved Hide resolved
@skjnldsv skjnldsv force-pushed the feat/addScript-dependency branch 3 times, most recently from b710397 to 4d970b7 Compare December 1, 2021 17:50
@skjnldsv skjnldsv requested a review from artonge December 1, 2021 17:50
@ChristophWurst
Copy link
Member

This is still a very fragile system, isn't it? It only works because we include the scripts in an sync fashion, therefore causing a slow page load. Once we do the recommended async loading the order script tags won't matter anymore and the same problems show.

What part of viewer is it that other apps have to await before they can run? Does viewer do anything async in its initialization?

lib/public/Util.php Outdated Show resolved Hide resolved
@skjnldsv skjnldsv force-pushed the feat/addScript-dependency branch 2 times, most recently from 0c9a8c8 to b9c5ca7 Compare December 1, 2021 18:29
@skjnldsv
Copy link
Member Author

skjnldsv commented Dec 1, 2021

This is still a very fragile system, isn't it? It only works because we include the scripts in an sync fashion, therefore causing a slow page load. Once we do the recommended async loading the order script tags won't matter anymore and the same problems show.

What part of viewer is it that other apps have to await before they can run? Does viewer do anything async in its initialization?

Yes and no. On the frontend, viewer can then wait for the DOMContentLoaded event.
The main issues is that there is only two steps, initializing and initialized. So if we load everything in a proper order (Core, Files, Viewer, PDFViewer), then Viewer can to its own init after the page is loaded, solving any API order. You can check the other PRs, this seems to works quite well.

@skjnldsv
Copy link
Member Author

skjnldsv commented Dec 1, 2021

Once we do the recommended async loading the order script tags won't matter anymore and the same problems show

Well, this is an entirely different issue to address. We'd have to rebuild our entire way to think (how we design APIS, how we register namespaces, etc etc), maybe it's a good first step?
I'm open to suggestions 🤔

EDIT: @ChristophWurst So, we apparently already defer all scripts. And it works with this change. So I guess it's all good! :)
image

@skjnldsv skjnldsv force-pushed the feat/addScript-dependency branch 2 times, most recently from 3447c6c to db56bd2 Compare December 2, 2021 18:44
@skjnldsv
Copy link
Member Author

skjnldsv commented Dec 2, 2021

Tests fixed

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@ChristophWurst
Copy link
Member

I was more thinking of moving the script tags from head to body but 👍

@skjnldsv skjnldsv force-pushed the feat/addScript-dependency branch 2 times, most recently from 800c592 to 0e49593 Compare December 2, 2021 19:29
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

🐘

@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 Dec 2, 2021
@Pytal
Copy link
Member

Pytal commented Dec 2, 2021

Was commiting lib/composer/bamarni/composer-bin-plugin intentional? this was auto-added for me before and I had to remove it manually 🤷‍♂️

@ChristophWurst
Copy link
Member

It should not be in git. Run composer i --no-dev

@skjnldsv
Copy link
Member Author

skjnldsv commented Dec 2, 2021

It should not be in git. Run composer i --no-dev

Yes 😢
I hate this so much ^^'

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv merged commit 41b6052 into master Dec 2, 2021
@skjnldsv skjnldsv deleted the feat/addScript-dependency branch December 2, 2021 20:20
@ChristophWurst
Copy link
Member

Sad CI noise

@skjnldsv
Copy link
Member Author

skjnldsv commented Dec 2, 2021

mejo- added a commit to nextcloud/text that referenced this pull request Dec 7, 2021
Required by nextcloud/server#30015

Signed-off-by: Jonas Meurer <jonas@freesources.org>
mejo- added a commit to nextcloud/text that referenced this pull request Dec 14, 2021
Required by nextcloud/server#30015

Signed-off-by: Jonas Meurer <jonas@freesources.org>
mejo- added a commit to nextcloud/text that referenced this pull request Dec 15, 2021
Required by nextcloud/server#30015

Signed-off-by: Jonas Meurer <jonas@freesources.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement pending documentation This pull request needs an associated documentation update technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants