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

fix(assets): image service bundling for pre-rendered pages non-node runtime regression #9087

Merged
merged 1 commit into from
Nov 14, 2023
Merged

fix(assets): image service bundling for pre-rendered pages non-node runtime regression #9087

merged 1 commit into from
Nov 14, 2023

Conversation

alexanderniebuhr
Copy link
Member

@alexanderniebuhr alexanderniebuhr commented Nov 13, 2023 •
edited
Loading

Changes

Testing

  • the fix of the regression was tested manually and compared to 3.4.4 (known good) and 3.5.0 (known bad)
  • the content collection cache implications were not tested, hopefully ci catches them

Docs

  • changeset

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 13, 2023

🦋 Changeset detected

Latest commit: 6c3bd26

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

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

@alexanderniebuhr
Copy link
Member Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Nov 13, 2023
natemoo-re
natemoo-re approved these changes Nov 13, 2023
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.

Works for me! I think the experimental content collections cache flag already has a number of bugs, so I’d rather prioritize fixing the regression. If we do discover that this breaks the flag, we can find a workaround for that in a follow-up PR.

@ematipico
Copy link
Member

ematipico commented Nov 13, 2023 •
edited
Loading

The thing is, not tests were broken after the regression. How can we prevent this from happening again?

@alexanderniebuhr
Copy link
Member Author

alexanderniebuhr commented Nov 13, 2023 •
edited
Loading

The thing is, not tests were broken after the regression. How can we prevent this from happening again?

Please correct me if I'm wrong, but I think we would need to add a test using astro build, which we currently don't do for astro:assets, and IIFRC there was some issue with that?

And additionally we would need to be testing for a special config, where the service is set and the endpoint is set to an empty file, so that image optimization only works on compile time and not runtime for SSR.
The specific image config we would need is:

https://github.com/withastro/adapters/pull/58/files#diff-2000e4d0dc6dedaf9158c48cb3b4ac0e13f2d8c20f9bda52361ee2be734f7b74R23-R29

cc @Princesseuh

@Princesseuh
Copy link
Member

Princesseuh commented Nov 13, 2023 •
edited
Loading

We do have tests for prod SSR, what we don't have are tests that test the specific bundling behaviour of the image service since it (should be) irrelevant.

The tests didn't catch this because astro:assets does work perfectly fine with this regression, it breaks this specific use case that only affects non-Node adapters. It'd be happy to have a test for this in the Cloudflare adapter, though

@alexanderniebuhr alexanderniebuhr changed the title fix: image service bundling for pre-rendered pages regression fix(assets): image service bundling for pre-rendered pages non-node runtime regression Nov 13, 2023
@alexanderniebuhr
Copy link
Member Author

I will add a test in withastro/adapters#58

@alexanderniebuhr alexanderniebuhr merged commit b895113 into main Nov 14, 2023
13 checks passed
@alexanderniebuhr alexanderniebuhr deleted the nbhr/fix_image_service_bundling_for_pre-rendered_pages_regression branch November 14, 2023 06:53
@astrobot-houston astrobot-houston mentioned this pull request Nov 14, 2023
Merged
peng added a commit to peng/astro that referenced this pull request Nov 17, 2023
@peng
Merge branch 'main' into feat-add-warn
cd112c9 
* main:
  feat(i18n): add `Astro.currentLocale` (withastro#9101)
  [ci] release (withastro#9107)
  Add compatibility with cloudflare node (withastro#8925)
  [ci] format
  Cancel response stream when connection closes (withastro#9071)
  [ci] format
  feat(i18n): apply specific routing logic only to pages (withastro#9091)
  feat(dev-overlay): Hide plugins into a separate menu when there's too many enabled (withastro#9102)
  [ci] format
  Support Svelte 5 (experimental) (withastro#9098)
  [ci] release (withastro#9078)
  [ci] format
  Refactor shikiji syntax highlighting code (withastro#9083)
  [ci] format
  fix: Query params trigger the trailingSlash error in preview mode (withastro#9045)
  fix(assets): bundling regression for specific config on non-Node runtimes (withastro#9087)
natemoo-re pushed a commit that referenced this pull request Nov 22, 2023
@alexanderniebuhr @natemoo-re
fix(assets): bundling regression for specific config on non-Node runt…
51e4cbe 
…imes (#9087)
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

@ematipico ematipico ematipico approved these changes

Assignees
No one assigned
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

astro:assets image service bundle regression
4 participants
@alexanderniebuhr @ematipico @Princesseuh @natemoo-re