fix(react): update nextjs peer dep and re-structure playgrounds #361
Conversation
maoberlehner
left a comment
maoberlehner
left a comment
There was a problem hiding this comment.
I very much like the new naming for the latest Next version! But not quite sure about the change for the Next 13 playground.
Ideally, I think we should have a light e2e test suite for all playground apps at some point to ensure they keep working.
Additionally, it makes sense to keep old versions around to test stuff in case people on legacy versions report issues.
So maybe we should always keep the latest + n versions around (n depends on the versioning policies of the framework).
After writing this, if we decide we want to keep n versions, maybe we should keep the version name after all? And with each new version, we delete the oldest playground and add a new one?
|
@maoberlehner those are great points! Also @edodusi shared similar feedback. I pretty much agree with it, specially for the test suite would be great to keep n-versions. Personally, I feel we need a distinction between the playgrounds used as part of the test suite, and the ones merely intended to play around and manual testing (or maybe we don't, I'd like to know your thoughts on that). To clarify, this was intended as small "leave it better than you found it" improvement. I think, as part of the test suite revamp project, we could upgrade the setup, add multi-version testing playgrounds, etc, to all SDKs playgrounds. What do you think? If so, maybe in the scope of this PR we can:
Thoughts? |
Hehe, always coming after one, such improvements! 😅 LGTM |
This PR is being reviewed by Cursor BugbotDetailsYour team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team. To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
Bug: Client Component Wraps Document TagsThe Additional Locations (1) |
|
@maoberlehner I see your point, and that makes sense. If we intend them to be doubled-role, then let's remove the testing part 😄 . I went ahead and renamed them to |
@storyblok/astro
storyblok
@storyblok/eslint-config
@storyblok/js
storyblok-js-client
@storyblok/management-api-client
@storyblok/nuxt
@storyblok/react
@storyblok/region-helper
@storyblok/richtext
@storyblok/svelte
@storyblok/vue
commit: |
Bug: StoryblokProvider Wraps Root Elements IncorrectlyThe |
| @@ -1,9 +1,6 @@ | |||
| import { storyblokEditable } from '@storyblok/react/rsc'; | |||
| import { headers } from 'next/headers'; | |||
There was a problem hiding this comment.
@maoberlehner note for the review:
I'm unsure about what was the intention of this here. It made the playground break (both Next 15 and 16) as soon as you wanted to use the bridge for live editing, as for that you need the StoryblokProvider pattern, and that makes the component a client component.
I also realized that the playground didn't include StoryblokProvider, and bridge wasn't enabled. I updated to include that, according to how it's documented https://www.storyblok.com/docs/guides/nextjs
2 participants
After investigating, testing and playing with it it, seems adding the peer dep was all needed.
At the same time, I took the chance to rename the playgrounds and fix a few minor things:
next13-app-router->next-e2e-testing(it was the one used for it, so now it's clearer that it's the one used for testing, and should be changed only when automated tests change, not for manual testing - just as we have it in the Nuxt SDK)next15->next-latest(so we don't need to rename it in every major release. Also, upgraded to Next 16)next15-static->next-latest-static(same)Closes #360