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

Add types to parameters of script fetching algorithms #9530

Merged

Conversation

nicolo-ribaudo
Copy link
Contributor

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

This PR adds types to all the parameters of script fetching algorithms, and converts all their variables to camelCase instead of using spaces.

Fixes #7996


/webappapis.html ( diff )

@nicolo-ribaudo nicolo-ribaudo force-pushed the script-fetching-algorithms-signatures branch from 2c089be to d1c452c Compare July 18, 2023 10:43
@nicolo-ribaudo nicolo-ribaudo changed the title Script fetching algorithms signatures Add types to parameters of script fetching algorithms Jul 18, 2023
source Show resolved Hide resolved
@nicolo-ribaudo nicolo-ribaudo marked this pull request as draft July 18, 2023 15:20
@nicolo-ribaudo nicolo-ribaudo force-pushed the script-fetching-algorithms-signatures branch from d1c452c to b6dd0a4 Compare July 18, 2023 15:33
@nicolo-ribaudo nicolo-ribaudo marked this pull request as ready for review July 18, 2023 15:42
@nicolo-ribaudo nicolo-ribaudo force-pushed the script-fetching-algorithms-signatures branch from b6dd0a4 to 010758d Compare July 18, 2023 15:57
data-x="">sharedworker</code>", or "<code data-x="">serviceworker</code>", and the <var>top-level
module fetch</var> flag is set, then set <var>request</var>'s <span
data-x="">sharedworker</code>", or "<code data-x="">serviceworker</code>", and
<var>isTopLevel</var> is true, then set <var>request</var>'s <span
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was referring to a flag that didn't actually exist anymore.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Thank you so much for this work!

source Show resolved Hide resolved
source Show resolved Hide resolved
<span>environment settings object</span> <var>scriptSettings</var>, an <var>onComplete</var>
algorithm, and an optional <span data-x="fetching-scripts-perform-fetch">perform the fetch
hook</span> <var>performFetch</var>, run these steps. <var>onComplete</var> must be an algorithm
accepting null (on failure) or a <span>classic script</span> (on success).</p>
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of renaming the ESO arguments to fetchClient and settingsObject? I'm not sure myself.

Copy link
Contributor Author

@nicolo-ribaudo nicolo-ribaudo Jul 19, 2023

Choose a reason for hiding this comment

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

The ESO in the various algorithms are used in different ways, so it would make sense to name them based on usage:

  • as a request client -> fetchClient
  • as a script's settings object -> settingsObject

However, in many algorithms (such as fetch a classic script and fetch a classic worker-imported script) it's used as both.

Additionally, some algorithms have a moduleMapSettings which also ends up being used passed as a settingsObject to algorithms that have nothing to do with the module map.

I'll push a commit given precedence to settingsObject, let me know what you think about 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.

An alternative is to call it settingsObject when there is one, and outsideSettingsObject/insideSettingsObject when there is two (similar to https://html.spec.whatwg.org/#worker-processing-model).

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@domenic domenic merged commit 3185b37 into whatwg:main Jul 20, 2023
1 check passed
@nicolo-ribaudo nicolo-ribaudo deleted the script-fetching-algorithms-signatures branch July 20, 2023 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Editorial cleanups to fetching scripts
2 participants