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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

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! :)

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