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.

Sign up for GitHub

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

Jump to bottom

improve preview reliability #2739

Merged
merged 1 commit into from
Mar 9, 2022
Merged

improve preview reliability #2739

merged 1 commit into from
Mar 9, 2022

Conversation

FredKSchott
Copy link
Member

Changes

  • Improves some preview logic for performance and bug fixes
  • Fixes flakey CI tests where close() would resolve before the server actually closed

Testing

  • Covered by CI

Docs

  • N/A

@changeset-bot
Copy link

changeset-bot bot commented Mar 9, 2022 •
edited
Loading

⚠️ No Changeset found

Latest commit: 08f5514

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) test labels Mar 9, 2022
FredKSchott
FredKSchott commented Mar 9, 2022
View reviewed changes
packages/astro/src/core/preview/index.ts
@@ -30,7 +30,10 @@ export default async function preview(config: AstroConfig, { logging }: PreviewO
const trailingSlash = config.devOptions.trailingSlash
/** Base request URL. */
let baseURL = new URL(config.buildOptions.site || '/', defaultOrigin);

const staticFileServer = sirv(fileURLToPath(config.dist), {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix: we shouldn't create a new instance on every request. This should speed up performance considerably if a site requests many assets per page.

FredKSchott
FredKSchott commented Mar 9, 2022
View reviewed changes
packages/astro/src/core/preview/index.ts
case hasTrailingSlash && trailingSlash == 'never' && !isRoot:
err('Prohibited trailing slash');
break;
case !hasTrailingSlash && trailingSlash == 'always' && !isRoot:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was a bug, where every URL required a trailing slash which would have made URLs like /favicon.png fail

FredKSchott
FredKSchott commented Mar 9, 2022
View reviewed changes
packages/astro/src/core/preview/index.ts
@@ -122,7 +119,9 @@ export default async function preview(config: AstroConfig, { logging }: PreviewO
port,
server: httpServer!,
stop: async () => {
httpServer.close();
await new Promise((resolve, reject) => {
Copy link
Member Author

@FredKSchott FredKSchott Mar 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flakey CI fix 1)

FredKSchott
FredKSchott commented Mar 9, 2022
View reviewed changes
packages/astro/test/0-css.test.js
@@ -25,7 +25,7 @@ describe('CSS', function () {

// test HTML and CSS contents for accuracy
describe('build', () => {
this.timeout(30000); // test needs a little more time in CI
this.timeout(45000); // test needs a little more time in CI
Copy link
Member Author

@FredKSchott FredKSchott Mar 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flakey CI fix 2)

natemoo-re
natemoo-re approved these changes Mar 9, 2022
View reviewed changes
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catches! Approved.

@FredKSchott FredKSchott force-pushed the improve-preview-reliability branch from c295ac4 to 08f5514 Compare March 9, 2022 18:56
@FredKSchott FredKSchott merged commit 0077b4c into next Mar 9, 2022
@FredKSchott FredKSchott deleted the improve-preview-reliability branch March 9, 2022 18:56
natemoo-re added a commit that referenced this pull request Mar 9, 2022
@natemoo-re @matthewp
@boehs @github-actions @FredKSchott
merge next into main (#2705)
72c2c86 
* Unflag the static build (#2652)

* Unflag the static build

* Only set legacyBuild to false if experimentalSSR is true

* Use legacy build when we have to

* Put a few more tests into legacy mode

* Last two

* Make astro-basic use the legacy build

* Adds a changeset

* Mark the lit test as legacy

* Update yarn lock

* Update based on feedback

* Add --legacy-build flag

* Move astro-basic test to use static build (#2682)

* Move some tests over to the static build (#2677)

* Move some tests over to the static build

* Fix assets tests

* Fix the assets tests

* Fix for the client:only components

* Moves asset tests to the static build

* Move postcss test over to static build

* Bring back legacy build for astro-basic test

* Move astro-basic test to use static build

* Migrate more tests to the static build (#2693)

* fix: disable HMR during build (#2684)

* Migrate more tests to the static build

* Only prepend links in non-legacy mode

* Add the 0-css tests

* Convert all CSS tests to the static build

* Migrate Astro global tests

* Remove .only

* Fix static build tests

* Migrate a few more

* More tests

* Move the lit test back to legacy

* Increase the test timeout

Co-authored-by: Nate Moore <[email protected]>

* Improve `head` injection behavior (#2436)

* feat: add renderHead util to server

* feat: remove `layouts` from config, Vite plugin

* fix: improve head injection during rendering

* chore: update compiler

* fix: do not escape links

* chore: enter `pre` mode

* Replace `send` with `sirv` (#2713)

* remove send

* Create thick-ravens-chew.md

* I feel like I'm going to screw something up

* working finally!

* rewrite req.url

* Add tiny bit of doc

* Update .gitignore

Co-authored-by: Evan Boehs <[email protected]>

* Move remaining tests to the static build (#2712)

* Move lit test to the static build

* Migrate astro-env plugin to work in the static build

* Do not remove vite:define

* Adds a changeset

* Add a warning when passing the --experimental-static-build flag (#2718)

* Add a warning when passing the --experimental-static-build flag

* Disable the lint warning

* [ci] release (next) (#2721)

* [ci] release (next)

* chore: update changeset

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Nate Moore <[email protected]>

* 404 page (#2719)

* Fix: build to 404.html in the static build

* Adds a changeset

* fix pnpm install missing peer deps

* fix svelte version in workspace

* fix lockfile

* fix(webapi): add dev script

* improve preview reliability (#2739)

* improve preview reliability - fix broken tests

* shamefully hoist to unblock

* remove lit from test running

* chore: update lockfile

Co-authored-by: Matthew Phillips <[email protected]>
Co-authored-by: Evan Boehs <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Fred K. Schott <[email protected]>
SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023
@natemoo-re @matthewp
@boehs @github-actions @FredKSchott
merge next into main (withastro#2705)
d956bfb 
* Unflag the static build (withastro#2652)

* Unflag the static build

* Only set legacyBuild to false if experimentalSSR is true

* Use legacy build when we have to

* Put a few more tests into legacy mode

* Last two

* Make astro-basic use the legacy build

* Adds a changeset

* Mark the lit test as legacy

* Update yarn lock

* Update based on feedback

* Add --legacy-build flag

* Move astro-basic test to use static build (withastro#2682)

* Move some tests over to the static build (withastro#2677)

* Move some tests over to the static build

* Fix assets tests

* Fix the assets tests

* Fix for the client:only components

* Moves asset tests to the static build

* Move postcss test over to static build

* Bring back legacy build for astro-basic test

* Move astro-basic test to use static build

* Migrate more tests to the static build (withastro#2693)

* fix: disable HMR during build (withastro#2684)

* Migrate more tests to the static build

* Only prepend links in non-legacy mode

* Add the 0-css tests

* Convert all CSS tests to the static build

* Migrate Astro global tests

* Remove .only

* Fix static build tests

* Migrate a few more

* More tests

* Move the lit test back to legacy

* Increase the test timeout

Co-authored-by: Nate Moore <[email protected]>

* Improve `head` injection behavior (withastro#2436)

* feat: add renderHead util to server

* feat: remove `layouts` from config, Vite plugin

* fix: improve head injection during rendering

* chore: update compiler

* fix: do not escape links

* chore: enter `pre` mode

* Replace `send` with `sirv` (withastro#2713)

* remove send

* Create thick-ravens-chew.md

* I feel like I'm going to screw something up

* working finally!

* rewrite req.url

* Add tiny bit of doc

* Update .gitignore

Co-authored-by: Evan Boehs <[email protected]>

* Move remaining tests to the static build (withastro#2712)

* Move lit test to the static build

* Migrate astro-env plugin to work in the static build

* Do not remove vite:define

* Adds a changeset

* Add a warning when passing the --experimental-static-build flag (withastro#2718)

* Add a warning when passing the --experimental-static-build flag

* Disable the lint warning

* [ci] release (next) (withastro#2721)

* [ci] release (next)

* chore: update changeset

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Nate Moore <[email protected]>

* 404 page (withastro#2719)

* Fix: build to 404.html in the static build

* Adds a changeset

* fix pnpm install missing peer deps

* fix svelte version in workspace

* fix lockfile

* fix(webapi): add dev script

* improve preview reliability (withastro#2739)

* improve preview reliability - fix broken tests

* shamefully hoist to unblock

* remove lit from test running

* chore: update lockfile

Co-authored-by: Matthew Phillips <[email protected]>
Co-authored-by: Evan Boehs <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Fred K. Schott <[email protected]>
SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023
@natemoo-re @matthewp
@boehs @github-actions @FredKSchott
merge next into main (withastro#2705)
1f461cb 
* Unflag the static build (withastro#2652)

* Unflag the static build

* Only set legacyBuild to false if experimentalSSR is true

* Use legacy build when we have to

* Put a few more tests into legacy mode

* Last two

* Make astro-basic use the legacy build

* Adds a changeset

* Mark the lit test as legacy

* Update yarn lock

* Update based on feedback

* Add --legacy-build flag

* Move astro-basic test to use static build (withastro#2682)

* Move some tests over to the static build (withastro#2677)

* Move some tests over to the static build

* Fix assets tests

* Fix the assets tests

* Fix for the client:only components

* Moves asset tests to the static build

* Move postcss test over to static build

* Bring back legacy build for astro-basic test

* Move astro-basic test to use static build

* Migrate more tests to the static build (withastro#2693)

* fix: disable HMR during build (withastro#2684)

* Migrate more tests to the static build

* Only prepend links in non-legacy mode

* Add the 0-css tests

* Convert all CSS tests to the static build

* Migrate Astro global tests

* Remove .only

* Fix static build tests

* Migrate a few more

* More tests

* Move the lit test back to legacy

* Increase the test timeout

Co-authored-by: Nate Moore <[email protected]>

* Improve `head` injection behavior (withastro#2436)

* feat: add renderHead util to server

* feat: remove `layouts` from config, Vite plugin

* fix: improve head injection during rendering

* chore: update compiler

* fix: do not escape links

* chore: enter `pre` mode

* Replace `send` with `sirv` (withastro#2713)

* remove send

* Create thick-ravens-chew.md

* I feel like I'm going to screw something up

* working finally!

* rewrite req.url

* Add tiny bit of doc

* Update .gitignore

Co-authored-by: Evan Boehs <[email protected]>

* Move remaining tests to the static build (withastro#2712)

* Move lit test to the static build

* Migrate astro-env plugin to work in the static build

* Do not remove vite:define

* Adds a changeset

* Add a warning when passing the --experimental-static-build flag (withastro#2718)

* Add a warning when passing the --experimental-static-build flag

* Disable the lint warning

* [ci] release (next) (withastro#2721)

* [ci] release (next)

* chore: update changeset

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Nate Moore <[email protected]>

* 404 page (withastro#2719)

* Fix: build to 404.html in the static build

* Adds a changeset

* fix pnpm install missing peer deps

* fix svelte version in workspace

* fix lockfile

* fix(webapi): add dev script

* improve preview reliability (withastro#2739)

* improve preview reliability - fix broken tests

* shamefully hoist to unblock

* remove lit from test running

* chore: update lockfile

Co-authored-by: Matthew Phillips <[email protected]>
Co-authored-by: Evan Boehs <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Fred K. Schott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natemoo-re natemoo-re natemoo-re approved these changes

Assignees
No one assigned
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@FredKSchott @natemoo-re