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

Remove importance="high" #1014

Closed
wants to merge 11 commits into from
Closed

Remove importance="high" #1014

wants to merge 11 commits into from

Conversation

AIC-BV
Copy link
Contributor

@AIC-BV AIC-BV commented Nov 28, 2023

This property does not exist (anymore?)

This property does not exist (anymore?)
This property does not exist (anymore?)
@LukeTowers
Copy link
Member

It was originally added in octobercms/october#4285 and it looks like there's one other place where it might still be present in the codebase, could you take a look?

Looks like the spec was renamed and adjusted since then and now exists as Fetch Priority. Would you be willing to take a look at that and make any necessary adjustments to retain the original intention of the PR while following the newest standard?

@LukeTowers LukeTowers added maintenance PRs that fix bugs, are translation changes or make only minor changes Status: Revision Needed labels Nov 30, 2023
@AIC-BV
Copy link
Contributor Author

AIC-BV commented Dec 1, 2023

@LukeTowers I added fetchpriority how I interpreted it.
I opened a really large order which takes around 4 seconds to load according to the network tab.
Using the modified code, it took significantly shorter: 2.5 seconds to load.
Don't know if it is coincidence or not.

BUT
I kept getting the 'invalid security token' error very often while refreshing so this definitly needs some investigation of the maintainers to optimise this PR.

@AIC-BV AIC-BV marked this pull request as draft December 1, 2023 09:04
@AIC-BV
Copy link
Contributor Author

AIC-BV commented Dec 1, 2023

Also low support
https://caniuse.com/?search=fetchpriority

@bennothommo
Copy link
Member

Some thoughts on this:

  • async attribute on scripts will introduce issues with loading scripts out of order on some browsers. This will cause issues for some widgets that need to load base scripts before others, such as the Inspector and Media finder.
  • The best way of loading scripts in way that prevents stalling still remains loading them just before the closing </body> tag. I recall the only reason we haven't done that is because some widgets (and possibly user plguins) load JavaScript inline that requires certain scripts to be available immediately.
  • My understanding is that we can use rel="preload" for all stylesheets without the need for the rel="stylesheet" line before it for all browsers now.

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Dec 7, 2023

Must've done something wrong since its broken now 😅

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Dec 7, 2023

Should work fine
https://caniuse.com/?search=preload

<?php endforeach; ?>

<?php foreach ($scripts as $script): ?>
<script data-cfasync="false" src="<?= $script . '?v=' . $coreBuild; ?>" importance="high"></script>
Copy link
Member

Choose a reason for hiding this comment

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

data-cfasync is still required AFAIK is it not? It's to prevent CloudFlare's Rocket Loader from breaking the backend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't seem to be doing anything, nothing is broken without it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

It will break sites for people who use CloudFlare Rocket Loader, but ideally, I would like for us to not have platform-specific fixes in core code and delegate it to a plugin, if it all possible.

Copy link
Member

Choose a reason for hiding this comment

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

Normally we do handle it via a plugin (https://github.com/wintercms/wn-cloudflare-plugin/blob/main/Plugin.php#L43-L54) but these are different because they're manually included instead of going through the AssetMaker trait.

Copy link
Member

Choose a reason for hiding this comment

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

Is it worth us changing these to go through AssetMaker then?

Copy link
Member

Choose a reason for hiding this comment

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

@bennothommo if you feel like doing that, sure 😉

Copy link

This pull request will be closed and archived in 3 days, as there has been no activity in this pull request for the last 6 months.
If you intend to continue working on this pull request, please respond within 3 days.
If this pull request is critical for your business, please reach out to us at [email protected].

@github-actions github-actions bot added the stale Issues/PRs that have had no activity and may be archived label Jun 20, 2024
@github-actions github-actions bot closed this Jun 23, 2024
@AIC-BV
Copy link
Contributor Author

AIC-BV commented Jun 24, 2024

👀

@mjauvin mjauvin reopened this Jun 24, 2024
@LukeTowers
Copy link
Member

@bennothommo any thoughts on this?

@LukeTowers LukeTowers removed the stale Issues/PRs that have had no activity and may be archived label Jun 25, 2024
@bennothommo
Copy link
Member

I think we can certainly remove the importance attribute - it's not being used. I still think we need to keep the data-cfasync for now until I pull my finger out and sort out some way of allowing a plugin to add it, in which case we can delegate it to the plugin.

@AIC-BV
Copy link
Contributor Author

AIC-BV commented Jul 12, 2024

Added the data-cfasync back to the line

@AIC-BV AIC-BV marked this pull request as ready for review July 12, 2024 09:05
@LukeTowers
Copy link
Member

Replaced by #1022, thanks for your help @AIC-BV!

@LukeTowers LukeTowers closed this Jul 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance PRs that fix bugs, are translation changes or make only minor changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants