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

Editorial: refactor callers of internal module script graph fetching procedure #4713

Merged
merged 9 commits into from
Jun 21, 2019

Conversation

hiroshige-g
Copy link
Contributor

@hiroshige-g hiroshige-g commented Jun 17, 2019

This PR simplifies the call graph around internal module script graph fetching procedure.

This PR makes top-level entry points of module loading

  • Fetch a module script graph
  • Fetch an inline module script graph (introduced by this PR)
  • Fetch a dynamic module script graph (introduced by this PR)
  • Fetch a module worker script graph
  • The fetch and process the linked resource algorithm for modulepreload links

call fetch the descendants of and instantiate,
that calls fetch the descendants of,
that calls internal module script graph fetching procedure,
that calls fetch the descendants of recursively.

Previously, internal module script graph fetching procedure can be top-level or non-top-level,
and thus calls fetch the descendants of and instantiate or fetch the descendants of accordingly,
depending on the top-level module fetch flag.
This PR makes internal module script graph fetching procedure non-top-level only, and
merges the top-level parts into the top-level entry points of module loading listed above.

This PR slightly changes the timing of FinishDynamicImport on error in resolve a module specifier,
but this doesn't affect observable behavior so much,
because promises are anyway resolved asynchronously.


/links.html ( diff )
/scripting.html ( diff )
/webappapis.html ( diff )

@hiroshige-g hiroshige-g marked this pull request as ready for review June 17, 2019 22:50
@hiroshige-g
Copy link
Contributor Author

@domenic WDYT?

This was a preparation for import maps spec.
This turns out not strictly required by the import maps spec draft, but probably is helpful as a general refactoring.

(I also created a call graph chart but I'm not sure whether this image should be also added)
sdQZtkd9ZpCINQ80k7LfWCw

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.

This seems pretty nice. A couple small things:

  • Can you rewrap changed paragraphs using https://output.jsbin.com/maferi ?
  • Let's make the names more obvious. I suggest:
    • Fetch an external module script graph
    • Fetch an inline module script graph
    • Fetch an import() module script graph
    • Fetch a module worker script graph

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@hiroshige-g
Copy link
Contributor Author

Let's make the names more obvious.

Sounds good. Renamed.

Also rewrapped lines.

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.

OK, I pushed a few more changes, including the diagram, and it looks good to me now. @hiroshige-g can you check over my changes with your own review?

@hiroshige-g
Copy link
Contributor Author

Thanks! Your commits look good to me.

The diagram looks nice. How did you prepare the diagram?

@domenic
Copy link
Member

domenic commented Jun 21, 2019

I wrote out the SVG by hand. I find it relaxing sometimes :)

@domenic domenic merged commit 1ce97b6 into whatwg:master Jun 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants