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

Injected scripts not injected to non-local injected routes #7563

Closed
1 task
delucis opened this issue Jul 4, 2023 · 2 comments · Fixed by #7943
Closed
1 task

Injected scripts not injected to non-local injected routes #7563

delucis opened this issue Jul 4, 2023 · 2 comments · Fixed by #7943
Assignees
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority)

Comments

@delucis
Copy link
Member

delucis commented Jul 4, 2023

What version of astro are you using?

2.7.3

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

None

What package manager are you using?

pnpm

What operating system are you using?

macOS / Stackblitz

What browser are you using?

Firefox/Chrome/Safari

Describe the Bug

I reported injectScript not to be working for injected routes in #7203 which was fixed by #7262.

However, the fix only works for routes that are local to a project because it determines if something is an injected route by trying to match the resolved file paths of routes to the endpoint specified in user config. This works fine for local routes where injectRoute({ entryPoint: 'src/my-route.astro' }) and the file path of 'src/my-route.astro' will match, but fails for situations where the resolved path and the authored path don’t match 1:1, for example if the route comes from a third-party package like Starlight: injectRoute({ entryPoint: '@astrojs/starlight/index.astro' }).

This may also impact other cases where authored and on-disk file paths can differ, e.g. for Windows users.

For the Starlight case, @hippotastic tried adding some debug logging and could see that @astrojs/starlight/index.astro was trying to match against a path of file:///D:/Dev/starlight/packages/starlight/index.astro and therefore failing to match.

Maybe a more fundamental fix in createRouteManifest is needed to specify injected routes early and not try to guess based on file path later?

The reproduction demonstrates this bug with a monorepo with one package that provides the route and one package that imports and renders the route on a page — where injected styles/scripts load successfully — and injects the route, where injected resources don’t load.

Reproduction instructions

# install deps
pnpm i
# move to the Astro site package
cd packages/site
# start the dev server
pnpm dev

The initial page shows Tailwind styles and an alert() added using injectScript().

Navigate to the “Injected route” link to see the same page, which will be missing the injected Tailwind styles and the alert() call.

What's the expected result?

Scripts or styles added with injectScript work on injected routes.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-ssvscu-zyglof?file=README.md

Participation

  • I am willing to submit a pull request for this issue.
@natemoo-re
Copy link
Member

Hmm, this is a much trickier than I anticipated! It turns out that this codepath often doesn't have the full route manifest available to check if a route is injected or not... This might require a larger refactor to fix properly.

@delucis
Copy link
Member Author

delucis commented Jul 13, 2023

Adding an additional use case we hit via users of Starlight: enabling analytics via the static Vercel adapter also relies on injectScript so currently doesn't work for injected routes.

@natemoo-re natemoo-re added the - P4: important Violate documented behavior or significantly impacts performance (priority) label Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants