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

[FEAAS] Nextjs Image for FEAAS #1754

Merged
merged 5 commits into from
Mar 13, 2024
Merged

Conversation

art-alexeyenko
Copy link
Contributor

@art-alexeyenko art-alexeyenko commented Mar 4, 2024

Description / Motivation

This change will ensure nextjs's implementation for Image is used when img elements are rendered withing FEAAS Components (from Component Builder). It also adds remotePatterns presets for dev and prod in nextjs.config - to allow easier management of rules for editing/dev and live deployments.

Note: FEAAS call is put into a separate component so as to not bloat Scripts.tsx. The feaas logic also doesn't belong under /byoc folder - so we're not putting it in byoc/index either.

Testing Details

  • Unit Test Added
  • Manual Test/Other (Please elaborate) - editing, connected, production modes

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@art-alexeyenko art-alexeyenko requested review from a team and Inviz March 4, 2024 21:59
@Inviz
Copy link
Contributor

Inviz commented Mar 12, 2024

Good progress forward, but we need to ensure that we are not dropping the ball on non-whitelisted images. As we're building a foolproof drop-in solution, and we need to balance the security concerns... we at least need to do the best effort. In this case, it should be not discarding the image.

@art-alexeyenko
Copy link
Contributor Author

Short update on implementation: we cannot match nextjs's matching logic without an extra dependency.
So the plan is this: we do limited matching for now. I will submit a PR for Vercel so images allow fallback to their unoptimized state if remotePatterns don't match. If and when it is released we revisit this implementation and simplify it.

@art-alexeyenko art-alexeyenko requested a review from ambrauer March 13, 2024 14:03
Copy link
Contributor

@illiakovalenko illiakovalenko left a comment

Choose a reason for hiding this comment

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

I approved, looks good 👍
Wait for Adam's approve and feel free to merge

Copy link
Contributor

@ambrauer ambrauer left a comment

Choose a reason for hiding this comment

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

Looks great! 👍

@art-alexeyenko art-alexeyenko merged commit 833eb02 into dev Mar 13, 2024
1 check passed
@art-alexeyenko art-alexeyenko deleted the feature/JSS-1616-image-in-feaas branch March 13, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants