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

Propagate the fetch settings object when loading module graphs #9520

Merged
merged 3 commits into from
Jul 18, 2023

Conversation

nicolo-ribaudo
Copy link
Contributor

@nicolo-ribaudo nicolo-ribaudo commented Jul 14, 2023

Fixes #9513 by passing the fetch client settings object through the ECMA-262 traversal algorithm, so that it is available when loading the individual modules.

The first two commits revert #9508 and #9510, while the third one is the actual fix.

cc @domenic @domfarolino @allstarschh


/webappapis.html ( diff )

@nicolo-ribaudo
Copy link
Contributor Author

nicolo-ribaudo commented Jul 14, 2023

After this PR I will open one to replace all the "settings object" parameters of the various loading-related algorithms with "settingsObject", because the casing inconsistency with the other parameters of these algorithms is annoying me a lot.

@allstarschh
Copy link

So I have one question about this PR.
The fetch client settings object in fetch the descendants of and link a module script, is passed as state.[[SettingsObject]], which is passed to LoadRequestedModules

And in LoadRequestedModules, the argument state is passed as [[HostDefined]] to HostLoadImportedModule

HostLoadImportedModule, HostLoadImportedModule(referrer, moduleRequest, loadState, payload)

So the original fetch client settings object is in the argument loadState, right?

But in HostLoadImportedModule,
Step 6, step 1-2,

Set settingsObject to referencingScript's settings object.

settingObject will be set to the refer's settings object, not the loadState.

So to fetch the descendants of a module script graph, it will use the settings object from the top-level script, instead of the original fetch client settings object.

Also see my previous comment in #9499 (comment)

@nicolo-ribaudo
Copy link
Contributor Author

loadState is only defined for graph loads initiated by HTML (i.e. <script> tags) and not for those initiated by ECMA-262 (i.e. dynamic import()).

With this PR, step 11.2 of HostLoadImportedModule gets settingsObject from loadState, when it is defined. Step 1 is a "default value" needed as a fallback for the import() case with no referrer (i.e. <button onclick="import('./a')"></button>), and step 6.2 overwrites it with a fallback default value for all the other dynamic import() cases.

@allstarschh
Copy link

With this PR, step 11.2 of HostLoadImportedModule gets settingsObject from loadState, when it is defined.

Oh, thanks for pointing that out, I didn't notice step 11.

@domenic
Copy link
Member

domenic commented Jul 18, 2023

Thanks so much! In case you're curious about the force-push I added "fixes #9513" to the commit message of the last commit so we have that in history.

After this PR I will open one to replace all the "settings object" parameters of the various loading-related algorithms with "settingsObject", because the casing inconsistency with the other parameters of these algorithms is annoying me a lot.

Yes please! (Feel free to camel-case any other parameters and variable names in these algorithms while you're there.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Module worker descendant script fetching regression
3 participants