-
-
Notifications
You must be signed in to change notification settings - Fork 99
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
fix(partially-fix-cold-start): add another component as layer for lazy loading #1121
base: master
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: dbb4efd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
✅ Deploy Preview for studio-next ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for asyncapi-studio-design-system ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for modest-rosalind-098b67 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @jerensl 👏 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the result and it seems like it is working. the only issue that I see here is the layers that we are adding. StudioEditor -> StudioWrapper -> CodeEditor. do you think we can do something about it? maybe combine them into one wrapper? or at least two?
if that's not possible, can you also add a comment on why this wrapper has been created?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason we added StudioEditor
is that we are unable to make root-level pages(/) as client components, so we added an extra component as a client component to call the StudioWrapper as code splitting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one last thing. can you also add a changeset so it triggers the release process?
EDIT: typo
@jerensl you don't need to version the PR. all that is required is having a changeset file. the CI/CD pipeline will take care of versioning. you can do that by running |
Thanks, I made the changes in the latest commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The sonar cloud status is not being reported and I don't see a comment from their bot eigther. seems like Sergio has found a way to resolve this issue here: asyncapi/parser-js#1033 (comment) @derberg can you try doing disable/enable branch protection rule for sonar to see if it works for us? EDIT: it seems like it doesn't work in other repos. |
UpdatedIn NextJS 15, the cache is no longer default. I'm also testing it on my deployment https://jolly-toffee-18b732.netlify.app/, and the cache lives longer. I assume that NextJS tries to invalidate the cache when it is already being cached by Netlify and did the same cache as well this led to a cold start because it stale while revalidated always looking at the first cache-store. So I will mark this as partially fixed, the rest will be done by updating to NextJS 15 when it's released, from now it's still on the release candidate |
Quality Gate passedIssues Measures |
Can Studio owners prioritize this issue? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Quality Gate passedIssues Measures |
Using
next/dynamic
in server components that loading a client component is still including everything in server side rendering, and that client component doesn't know it needs to be split into another chunk in browser chunks.Related: vercel/next.js#49454 (comment)
Description
I set up my own deployment here https://jolly-toffee-18b732.netlify.app and tested the results below
Related issue(s)
Partially fix #1118