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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[cli] Add tests to compare dev/prd image optimization #5428

Merged
merged 7 commits into from
Nov 16, 2020

Conversation

styfle
Copy link
Member

@styfle styfle commented Nov 16, 2020

Add E2E test for to test image optimization against vc dev as well as a prod deployment.

馃搵 Checklist

Tests

  • The code changed/added as part of this PR has been covered with tests
  • All tests pass locally with yarn test-unit

Code Review

  • This PR has a concise title and thorough description useful to a reviewer
  • Issue from task tracker has a link to this PR (CH13104)

@styfle styfle changed the title Ch13104/e2e img tests Add dev/prd E2E tests for image optimization Nov 16, 2020
@styfle styfle changed the title Add dev/prd E2E tests for image optimization [cli] Add tests to compare dev/prd image optimization Nov 16, 2020
Copy link
Contributor

@juancampa juancampa left a comment

Choose a reason for hiding this comment

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

@styfle does vc dev return last-modified? That's something that people have asked about, imgix returns it and we simply forward it

@styfle
Copy link
Member Author

styfle commented Nov 16, 2020

@juancampa No it does not. The next dev implementation uses etag instead (per @Timer) which is why it was omitted from these tests. See vercel/next.js#18986

@juancampa
Copy link
Contributor

Got it, that's an inconsistency that we might want to fix later

juancampa
juancampa previously approved these changes Nov 16, 2020
@styfle styfle added the semver: patch PR contains bug fixes label Nov 16, 2020
@Timer
Copy link
Member

Timer commented Nov 16, 2020

@juancampa I think the inconsistency is inherent to the fact that Next.js is optimizing the image itself (thus has the Etag readily at its disposal) verses on Vercel where we proxy Imgix. I don't see the need to be consistent here.

@juancampa
Copy link
Contributor

I could see users being confused by etags appearing in their vc dev experience, but not in prod. But yeah, for now I think it's okay to accept that there are some (non fundamental) differences between the two environments

"version": 2,
"build": {
"env": {
"FORCE_BUILDER_TAG": "canary"
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Member Author

@styfle styfle Nov 16, 2020

Choose a reason for hiding this comment

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

Its not necessary but it will help us catch differences in @vercel/next canary before they go to stable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants