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

🐛 BUG: Partytown integration does not work with some SSR adaptors #3668

Closed
1 task done
nrgnrg opened this issue Jun 21, 2022 · 9 comments · Fixed by #3686
Closed
1 task done

🐛 BUG: Partytown integration does not work with some SSR adaptors #3668

nrgnrg opened this issue Jun 21, 2022 · 9 comments · Fixed by #3686
Assignees
Labels
- P3: minor bug An edge case that only affects very specific usage (priority)

Comments

@nrgnrg
Copy link
Contributor

nrgnrg commented Jun 21, 2022

What version of astro are you using?

^1.0.0-beta.47

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

netlify, cloudflare

What package manager are you using?

pnpm

What operating system are you using?

Mac

Describe the Bug

Both the Netlify and Cloudflare adaptors serve static assets by deferring the request to the platform, they do this by first checking if the request is for something that exists in the manifest from the createExports args.

// netlify adaptor
if (manifest.assets.has(url.pathname)) {
  return;
}

// cloudflare adaptor
if (manifest.assets.has(pathname)) {
  const assetRequest = new Request(`${origin}/static${pathname}`, request);
  return env.ASSETS.fetch(assetRequest);
}

the manifest.assets.has(pathname) fails for partytown assets because the partytown static assets do not exist in the manifest and thus they 404.

I'm not sure wether this is a bug in the partytown integration or wether the adaptors should handle this better.

Partytown could be special cased in the adaptors or the check could be more forgiving with a regex like so: /\.\w+$/.test(pathname). Although both are a workaround.

Link to Minimal Reproducible Example

I tried to set this up in a repro but was having difficulties, I can give it another go if this isn't clear enough.

Participation

  • I am willing to submit a pull request for this issue.
@bholmesdev bholmesdev added - P3: minor bug An edge case that only affects very specific usage (priority) s1-small labels Jun 22, 2022
@bholmesdev
Copy link
Contributor

Thanks for the thorough explanation @nrgnrg! I agree this should be handled better on the integration side. Personally, I think adding partytown assets to the manifest is worth investigating over the workaround you mentioned. We'd need to investigate how possible this is though.

@matthewp matthewp self-assigned this Jun 22, 2022
@matthewp
Copy link
Contributor

@nrgnrg can you try this with astro@1.0.0-beta.52?

@nrgnrg
Copy link
Contributor Author

nrgnrg commented Jun 22, 2022

@matthewp looks like this is still an issue astro@1.0.0-beta.52

@matthewp
Copy link
Contributor

It's possible that type of integration does not currently work in SSR. I'll look into it.

@nrgnrg
Copy link
Contributor Author

nrgnrg commented Jun 22, 2022

I think the issue is here

await copyLibFiles(fileURLToPath(new URL('~partytown', dir)), {
debugDir: false,
});

it copies over the files into to the build folder on astro:build:done so there not included in the manifest and thus the SSR adaptors don't know how to route requests for them

@matthewp
Copy link
Contributor

@nrgnrg do they get added to the page though? And 404 from there?

@nrgnrg
Copy link
Contributor Author

nrgnrg commented Jun 22, 2022

it looks like a script tag gets added to the head which I think is initiating the request

@matthewp
Copy link
Contributor

PR: #3686

@nrgnrg
Copy link
Contributor Author

nrgnrg commented Jun 22, 2022

amazing! 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P3: minor bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants