Determine hoisted scripts via AST analysis

[ottomated]

Changes

• Fixes Hoisted scripts break when components are re-exported from a typescript file #7673
• Previous behavior: If a component is anywhere in the import tree of the current page, include its script in the hoisted bundle
• New behavior: Components must be actually imported by the current page for their hoisted scripts to be included.

TODO

• Investigate performance - there are opportunities for early returns and caching.
• Attempt to move MDX-specific logic out of the core.
• Add documentation
• Make this work for <style> tags as well
• This will current break in the case where a component is imported by another component. Fix would be to switch import tracking to the importer in this case?
• Handle dynamic imports
• depthsToExportNames should be a Map<number, string[]> because technically the same component could be exported twice from the same file under different names

Testing

A new test was added that tests various combinations of re-exporting style and checks to see if the built pages contain the expect components and no others.

Docs

TODO: document the exact behavior in the hoisted scripts section

/cc @withastro/maintainers-docs for feedback!

[changeset-bot]

🦋 Changeset detected

Latest commit: 097ed48

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[ottomated]

This would break the edge case where a component is exported indirectly, like so:

import Component from './Component.astro';

export default getComponent() {
  return Component;
}

Not sure if that's an issue or not. It would require much deeper code analysis, tracking the imported variables, to solve that.

[matthewp]

As long as this doesn't cause regressions I think a small change to support the common re-export pattern makes sense. I would just want the code cleaned up a little bit, move the AST logic into a separate, clearly named function.

[ottomated]

Last change is the component tracking switcher needs to handle MDX. @matthewp which file extensions specifically can render astro components? Is it just .astro and .mdx?

[ottomated]

I assume this is unacceptable - mdx logic shouldn't be in the core. Unsure on how to resolve.

[matthewp]

Yeah, given we already have this in the code. Not ideal.

[bluwy]

I haven't tested this yet, but shouldn't the ast already contain these exports and added them to exportNames?

[ottomated]

This is to handle a slightly different case. If component Title.astro is imported and used by TitleBar.astro, TitleBar.astro is not going to re-export Title. Therefore, this line basically switches from tracking Title to TitleBar, so the page will include Title's scripts.

[lilnasy]

If we only need to scan the imports, maybe we can avoid creating and walking the full AST by using es-module-lexer instead.

[ottomated]

If we only need to scan the imports, maybe we can avoid creating and walking the full AST by using es-module-lexer instead.

Well, the AST is already created, but this is worth looking into I suppose

Edit: es-module-lexer does not return the names of what is imported, just where they're imported from. It won't work for this case.

[matthewp]

This looks really good! I'm going to update the branch to see if that kicks off CI to run fully.

[bluwy]

Clean tests! I think our strategy for hoisting by statically analyzing things won't always be correct, so having a best-effort implementation for now is fine by me.

There would be a slight performance hit with this as reading the ast in Rollup introduces an acorn parse, but running some build tests doesn't yield a big time difference, so maybe it is negligible.

[yanthomasdev]

Thanks, this looks great. Just found a small typo in the changeset! 🙌

[ottomated]

Do we want to make this also work for styles before merging? It shouldn't be that hard

[sarah11918]

Hi @matthewp !

Now that Yan has made the minor fixes, this would be fine as-is after making <script> plural in line 7 (either with an "s" or "script TAGS")... I just think it could be better! ;)

See my updated take for your consideration and the reasons I'd be inclined to restructure. But totally your call! Take what you like from it, or take nothing at all!

[ottomated]

The existing approach actually did work in most cases (delivering different bundles per page) - this change only fixes the re-export case below.

[sarah11918]

@ottomated Can you suggest a rewording that is true? What would this paragraph have to look like so that it is accurate?

[ottomated]

reworded in #7733 !