Skip to content

Add types to parameters of script fetching algorithms#9530

Merged
domenic merged 6 commits into
whatwg:mainfrom
nicolo-ribaudo:script-fetching-algorithms-signatures
Jul 20, 2023
Merged

Add types to parameters of script fetching algorithms#9530
domenic merged 6 commits into
whatwg:mainfrom
nicolo-ribaudo:script-fetching-algorithms-signatures

Conversation

@nicolo-ribaudo

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

Copy link
Copy Markdown
Member

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
Comment thread source Outdated
@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
Comment thread source
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
Copy Markdown
Member 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.

@domenic domenic left a comment

Copy link
Copy Markdown
Member

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!

Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
<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
Copy Markdown
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.

@nicolo-ribaudo nicolo-ribaudo Jul 19, 2023

Copy link
Copy Markdown
Member Author

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
Copy Markdown
Member 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).

Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
Comment thread source Outdated
@domenic domenic merged commit 3185b37 into whatwg:main Jul 20, 2023
@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