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

fix: --experimental-ssr fixes #2937

Merged
merged 14 commits into from
Mar 31, 2022
Merged

fix: --experimental-ssr fixes #2937

merged 14 commits into from
Mar 31, 2022

Conversation

JuanM04
Copy link
Contributor

@JuanM04 JuanM04 commented Mar 29, 2022

Changes

Replaced --experimental-ssr with the internal function isBuildingToSSR, which reads the brand-new adapter inside the Astro config.

  • Use isBuildingToSSR() everywhere.
  • --experimental-ssr flag now is required when using 3rd-party adapters.

This change has a side effect: it now respects the output given by the integrations through buildConfig and (somehow) doesn't copy the contents of public/ into the dist folder. @matthewp maybe you can help me with this?

Testing

TBD

Docs

TBD

@changeset-bot
Copy link

changeset-bot bot commented Mar 29, 2022

🦋 Changeset detected

Latest commit: cb45642

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

This PR includes changesets to release 1 package
Name Type
astro Patch

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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pkg: example Related to an example package (scope) test labels Mar 29, 2022
@JuanM04 JuanM04 mentioned this pull request Mar 30, 2022
@matthewp
Copy link
Contributor

Hm, so you're saying that public/ works before this change with the flag but fails just by using this internal code instead? I'm not sure why that would be, you might need to debug it more. I believe Vite is the one doing the copying there, with the publicDir config, maybe that is being messed up some how?

@JuanM04
Copy link
Contributor Author

JuanM04 commented Mar 30, 2022

Things were broken in a different way before - for example, now buildConfig is respected.

Nonetheless, this new test also fails in the main branch.

@matthewp
Copy link
Contributor

Oh, I just read the test. You are expecting to get the public/ file by calling the adapter, but that test adapter doesn't implement static file serving. That would be a feature of the adapter, not a feature of our SSR implementation.

@JuanM04
Copy link
Contributor Author

JuanM04 commented Mar 30, 2022

That would be a feature of the adapter, not a feature of our SSR implementation.

If you clone this PR and run the following:

pnpm install
pnpm build
cd examples/blog
pnpm astro add node --yes
pnpm build

You'll see the contents of the public folder inside dist/client. Nonetheless, if you run the same steps inside examples/minimal, dist/client/ is empty. That's what I'm referring to

You are expecting to get the public/ file by calling the adapter, but that test adapter doesn't implement static file serving.

Ohhh, I see. I removed that test, but the problem described above persists

@matthewp
Copy link
Contributor

Yeah, perhaps something about minimal is causing a bug then? I'm not sure.

Talked about this overall idea with the team and we want to make it so that the flag is still required if using a 3rd-party adapter, but not needed if using a built-in adapter.

This mimics how integrations work, where the flag is needed if using a 3rd party one.

@JuanM04
Copy link
Contributor Author

JuanM04 commented Mar 31, 2022

we want to make it so that the flag is still required if using a 3rd-party adapter

Perfect, I could add that to isBuildingToSSR!

Yeah, perhaps something about minimal is causing a bug then? I'm not sure.

After some searching, I think the issue is here. When there is no JavaScript to be generated, the build process is omitted. This leads to sites with static assets (and probably some CSS) to not have its assets copied if there is no JS.

@matthewp
Copy link
Contributor

After some searching, I think the issue is here. When there is no JavaScript to be generated, the build process is omitted. This leads to sites with static assets (and probably some CSS) to not have its assets copied if there is no JS.

Ah, yeah that does make sense. I think I can fix that in another PR.

@matthewp
Copy link
Contributor

Here's how the integrations flag is implemented: 9d6e0b5

@JuanM04 JuanM04 changed the title fix: replaced --experimental-ssr with isBuildingToSSR fix: --experimental-ssr fixes Mar 31, 2022
@matthewp
Copy link
Contributor

Code looks good, maybe need to rebase with main to clear up the failing test?

JuanM04 and others added 3 commits March 31, 2022 14:09
@matthewp
Copy link
Contributor

Great, thank you!

@matthewp matthewp merged commit d10c3de into main Mar 31, 2022
@matthewp matthewp deleted the fix/ssr-experimental-flag branch March 31, 2022 17:58
@github-actions github-actions bot mentioned this pull request Mar 31, 2022
SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023
* Replaced `--experimental-ssr` with `isBuildingToSSR`

* changest

* Improved `isBuildingToSSR` a bit

* Added `isBuildingToSSR` to more places!!1!

* Added `@deprecated` tag

* Replaced missing experimentalSsr

* Added failing test

* Removed test

* Re-added experimental ssr flag

* Fixed typo

Co-authored-by: Matthew Phillips <matthew@skypack.dev>

* Fixed deno tests

Co-authored-by: Matthew Phillips <matthew@skypack.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pkg: example Related to an example package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants