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

feat(cli): Setup command and codemod for OG image middleware #10485

Merged
merged 15 commits into from May 7, 2024

Conversation

Josh-Walker-GM
Copy link
Collaborator

@Josh-Walker-GM Josh-Walker-GM commented Apr 19, 2024

This PR introduces a setup command for the OG image generation middleware.


It did involve moving around some of the codemod utilities we have. Either because we needed to include them where we previously didn't or because they no longer made sense in the directory they were in.

@Josh-Walker-GM Josh-Walker-GM added release:feature This PR introduces a new feature changesets-ok Override the changesets check labels Apr 19, 2024
@Josh-Walker-GM Josh-Walker-GM added this to the SSR milestone Apr 19, 2024
@Josh-Walker-GM Josh-Walker-GM self-assigned this Apr 19, 2024
@Josh-Walker-GM
Copy link
Collaborator Author

@cannikin Two points to add when you go to look at this:

  1. I noticed that the OG image middleware package doesn't actually export the things being imported here. I think this is currently being introduced in feat(og-gen): Implement middleware and hooks  #10469. We should ensure that whatever the exports end up looking like that the imports are consistent with them.
  2. I see that this feature is still "experimental"? Currently, this PR puts the setup command in the stable cli section. If you want to move it then I'm fine with that. My only concern is that we keep the experimental CLI flat so we would be undoing the middleware directory and just have it be a single command within the experimental directory. If that makes any sense?

@Josh-Walker-GM
Copy link
Collaborator Author

I also noticed the codemod tests don't cover the case where the middleware is already setup. After thinking about it for a moment, I don't want to consider this case. The reward from doing so given the complexity required to determine if that is the case is too low in my opinion.

@cannikin
Copy link
Member

I'm okay with it being yarn rw setup middleware ogimage as I think we'll have other ones that use this going forward, maybe even before SSR is ready in a regular Redwood release.

Even though SSR support is experimental, the ogimage stuff should be stable and ready to go. Maybe there's a check that if you run that command, but SSR hasn't already been enabled, it errors and lists the command to run to install it? yarn rw setup experimental setup-streaming-ssr

@cannikin
Copy link
Member

I also noticed the codemod tests don't cover the case where the middleware is already setup. After thinking about it for a moment, I don't want to consider this case. The reward from doing so given the complexity required to determine if that is the case is too low in my opinion.

That's fine, as long as it just stops and doesn't end up mangling the file because it couldn't figure out what to do? Can it just error out, say something about the file contents are unexpected, and give a sample output of the import and what registerMiddleware() needs to include to get it to work? Then the user can edit themselves.

@dac09
Copy link
Collaborator

dac09 commented Apr 23, 2024

Nice one @Josh-Walker-GM - I think missing step is updating Vite.config.js to include the plugin too! :)

@Josh-Walker-GM Josh-Walker-GM marked this pull request as ready for review May 6, 2024 00:31
Copy link
Member

@cannikin cannikin left a comment

Choose a reason for hiding this comment

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

Looking good! Can we add post-setup instructions:

og:image generation is almost ready to go! You'll need to add 
playwright as a dependency to the api side and then install the 
headless browser packages:

  yarn workspace api add playwright
  cd api
  yarn playwright install

Depending on how your host is configured you may need to 
install additional dependencies first. If so, the `playwright install` 
step will error out and give you the command to run to install 
those deps.

@Josh-Walker-GM Josh-Walker-GM added changesets-ok Override the changesets check and removed changesets-ok Override the changesets check labels May 7, 2024
@cannikin
Copy link
Member

cannikin commented May 7, 2024

I forgot to test this, but should this script (or maybe it does already?) error out if you try to setup og-image but haven't already setup-experimental-ssr? Like you won't have entry.server.js if you haven't setup SSR yet. It probably already errors, but does the error message tell you how to resolve?

@Josh-Walker-GM
Copy link
Collaborator Author

Josh-Walker-GM commented May 7, 2024

I forgot to test this, but should this script (or maybe it does already?) error out if you try to setup og-image but haven't already setup-experimental-ssr? Like you won't have entry.server.js if you haven't setup SSR yet. It probably already errors, but does the error message tell you how to resolve?

Yeah the first task in the command handler checks that you have setup streaming ssr.

title: 'Check prerequisites',
skip: force,
task: () => {
if (!getConfig().experimental?.streamingSsr?.enabled) {
throw new Error(
'The Streaming SSR experimental feature must be enabled before you can setup middleware',
)
}
},

@Josh-Walker-GM Josh-Walker-GM enabled auto-merge (squash) May 7, 2024 18:32
@Josh-Walker-GM Josh-Walker-GM merged commit 7c98a41 into main May 7, 2024
46 checks passed
@Josh-Walker-GM Josh-Walker-GM deleted the jgmw/og-image-setup-command branch May 7, 2024 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changesets-ok Override the changesets check release:feature This PR introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants