walkParentInfos handles circular imports too naively

[ottomated]

What version of astro are you using?

2.9.5

Are you using an SSR adapter? If so, which one?

N/A

What package manager are you using?

npm

What operating system are you using?

Linux

What browser are you using?

Chrome

Describe the Bug

When analyzing hoisted script imports, the following situation can happen:

graph TD;
    Component["/src/components/Component.astro"]
    Layout["/src/components/Layout.astro"]
    index["/src/components/index.ts"]
    bad["/src/pages/bad.astro"]
    good["/src/pages/good.astro"]
    Component-->Layout;
    Layout-->index;
    index-->bad;
    index-->good;

Loading

This causes an issue when bad.astro has import { Component } from '../components';. This is because the path from Component.astro -> index.ts -> bad.astro is never traversed, meaning Astro can't analyze the import.

What's the expected result?

The expected traversal is:

graph TD;
    Component["/src/components/Component.astro"]
    Layout["/src/components/Layout.astro"]
    index["/src/components/index.ts"]
    bad["/src/pages/bad.astro"]
    good["/src/pages/good.astro"]
    Component-->Layout;
    Layout-->index;
    Component-->index;
    index-->bad;
    index-->good;

Loading

Since walkParentInfo's implementation prevents circular imports by ensuring each module is only visited once, we can't to visit index.ts twice like we need to.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-xidexe?file=dist%2Fbad%2Findex.html

Participation

• I am willing to submit a pull request for this issue.

[natemoo-re]

Thanks for opening an issue, @ottomated! Is this something you're planning to tackle in #7733 ?

[ottomated]

@natemoo-re I think it should be done on that branch, but I don't think I'm the best person for the job – I assume there's an existing algorithm for resolving circular imports that someone else would know.

[natemoo-re]

@natemoo-re I think it should be done on that branch, but I don't think I'm the best person for the job – I assume there's an existing algorithm for resolving circular imports that someone else would know.

Cool, thanks for the context! @bluwy hopefully might know something of the technique Vite uses to handle this...

[bluwy]

I took a look at the issue in the past few days. I think I got a good grasp of it now, and what proposed here would indeed improve the optimization.

But looking into all the edge case surrounding hoisted scripts, it's a lot complex than expected.

1. The optimization detection misses export default { Tabs, Grid } (object form, or any complex shape). It would be hard to track references with only the AST. (There's periscopic but I feel like there will be more)
2. We want different behaviours for dynamic imports:
   • For .../index.astro?astroPropagatedAssets that dynamically imports .../index.astro, we want the former to inherit the latter's heuristic.
   • For astro:content virtual module, which dynamically imports all assets, any other module that imports astro:content will be mark as parent even though they don't actually use the dynamically imported assets.
   • Fixing both makes the behaviour unpredictable when user scales their code.

I think it will be most robust if scripts (and styles?) are added during render time, instead of build time. Build-time analysis can only go very far and usually relies on side-effects, which is what we're emulating before for scripts (and styles). For now, I'll revert #7707 to unblock the issue.

This would "dumb down" the hoisted script heuristic, which isn't great, but I think it's better to be predictable for now and find a better solution in the future.

[ottomated]

I completely agree that render-time is better than build-time for this (although two issues that instantly strike me are the need to bundle scripts, i.e. a build step after the render step, and what to do for SSR pages).

However, I would ask that we consider fixing these edge cases before reverting, or at least raising an issue with the same priority as #7744.

1. Your first point could be addressed fairly easily, and I can't think of another edge case that is a pattern anyone's actually using.
2. The current solution for dynamic imports is equivalent to the reverted version, so the point there is moot for now.
3. Including scripts that shouldn't be in the page is almost as bad as missing scripts that should be, it's just slightly less visible to the end user.

Otherwise, I would ask for this behavior to be behind an experimental flag for now so it doesn't entirely block my project.

[bluwy]

1. Your first point could be addressed fairly easily, and I can't think of another edge case that is a pattern anyone's actually using.

There could be a lot of patterns people are using: export const MarkdownComponents = { Heading }, export default { Foo: Bar }, or even things that are not static analyzable.

2. The current solution for dynamic imports is equivalent to the reverted version, so the point there is moot for now.

My point is the consistency where one page would sometime works and sometimes doesn't, e.g. .astro and .mdx are handled different where currently .mdx pages wouldn't work. Sometimes the barrel file gets refactored and it doesn't work, and it's hard to track this down and ask users for a repro when this happens.

Otherwise, I would ask for this behavior to be behind an experimental flag for now so it doesn't entirely block my project.

I'm open to that too. (I'm signing off for the day and call handle this tomorrow, otherwise feel free to send a PR too)