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

should keep script async attribute to false for block executing scripts #7

Open
kuitos opened this issue Nov 17, 2023 · 6 comments
Open

should keep script async attribute to false for block executing scripts #7

kuitos opened this issue Nov 17, 2023 · 6 comments

Comments

@kuitos
Copy link

kuitos commented Nov 17, 2023 •
edited
Loading

Version: 1.0.3

Details

I found that non-async scripts marked with defer will become async after insertion, which causes the defer scripts not to be executed in document order. There is no problem with non-defer synchronous scripts because we have a blocking flow that waits for the previous script to finish before loading the next one.

I did some digging and found that this is because scripts written through a documentFragment will all become async true, and the same issue exists with document.importNode.
Here is example code to explain the issue：

const doc = document.implementation.createHTMLDocument('');
doc.write('<!DOCTYPE html><body><template>');
const root = (doc.body.firstChild).content;
const walker = doc.createTreeWalker(root);
doc.write('<script src="./a.js"></script>');

console.log(walker.root.children[0].async);  // true
const script = document.createElement('script');
script.async = false;
const clone = document.importNode(script);
console.log(clone.async); // true

Expected Behavior

defer script without async attribute on html should execute in the order in which they appear in the document.

Actual Behavior

defer script without async attribute on html not executed in order

Possible Fix

set script element async attribute correctly before it been append to target document

writable-dom/src/index.ts

Line 121 in 020d50b

const parentNode = targetNodes.get(node.parentNode!)!; 

if (isSyncScript(clone)) {
  clone.async = false;
}

Your Environment

  • Chrome 119
  • MacOS 14.0
@kuitos kuitos changed the title should reset script async attribute to false for block executing scripts should keep script async attribute to false for block executing scripts Nov 17, 2023
@DylanPiercey
Copy link
Contributor

DylanPiercey commented Nov 18, 2023 •
edited
Loading

@kuitos curious what impact you are seeing from this behavior? Currently the blocking scripts are handled by writabledom itself without changing the underlying property (just uses attribute presence). Changing the property wouldn't really have an impact besides other code explicitly reading that async property?

(specifically here it uses an attribute check instead of a property check https://github.com/marko-js/writable-dom/blob/main/src/index.ts#L161)

@DylanPiercey
Copy link
Contributor

Oh my bad your issue is with "defer" scripts.
Yeah that isn't currently well supported, but probably should be.

FWIW I recommend avoiding defer scripts and instead preferring actual asyn scripts (especially when streaming)

@kuitos
Copy link
Author

kuitos commented Nov 20, 2023 •
edited
Loading

Yeah this issue is all about defer scripts.

FWIW I recommend avoiding defer scripts and instead preferring actual asyn scripts

But in my knowledge, some popular frontend frameworks used defer to improve there first screen performance by default, especially in streaming ssr mode, like vue and nextjs.

@DylanPiercey
Copy link
Contributor

The problem with defer is that these scripts cannot execute until the entire page is done, but with async you have (in a non blocking way) the script execute as soon as possible.

if the framework you're using supports async hydration then you should always use async over defer since the user will be able to interact with the page sooner.

Either way ideally this module would support defer.

It's a bit confusing as to way defer should do in a micro-frame though. Normally defer waits until DOMContentLoaded to execute, however I guess here it'd have to wait just until the writable dom instance has closed. One problematic piece here is that if you use micro-frame or another wrapper of this module which attempts to make it isomorphic there is no way I can think of to actually make the defer scripts operate that when when server rendering. Not that this is necessarily a big deal for this module.

@kuitos
Copy link
Author

kuitos commented Nov 21, 2023 •
edited
Loading

Normally defer waits until DOMContentLoaded to execute, however I guess here it'd have to wait just until the writable dom instance has closed.

Yes, that's what I was thinking. Another problem is how to block the execution of deferred scripts. My thought is to remove their src attribute and re-add it until the writable stream is closed.

One problematic piece here is that if you use micro-frame or another wrapper of this module which attempts to make it isomorphic there is no way I can think of to actually make the defer scripts operate that when when server rendering.

Sorry I am not familiar with micro-frame, I'm not sure is defer script will be used when server-side rendering? As far as I know, the defer script string is written to the HTML response during server rendering and then executed by the browser client, the server does not interact with the defer script in HTML stream, is my understanding correct?

@kuitos
Copy link
Author

kuitos commented Nov 23, 2023

If we don't consider fully consistent to browser behavior for now (defer scripts must execute before DOMContentLoaded but after HTML parsed), and only need to ensure that defer scripts execute in order, as long as they are inserted into the document with async set to false before execution, does this make sense?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@DylanPiercey @kuitos