Upgrade to next 13 #1726
Upgrade to next 13 #1726
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📦 Next.js Bundle Analysis for @ifixit/commerce-frontendThis analysis was generated by the Next.js Bundle Analysis action. 🤖 🎉 Global Bundle Size Decreased
DetailsThe global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster. Any third party scripts you have added directly to your app using the If you want further insight into what is behind the changes, give @next/bundle-analyzer a try! Eight Pages Changed SizeThe following pages changed size from the code in this PR compared to its base branch:
DetailsOnly the gzipped size is provided here based on an expert tip. First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If Any third party scripts you have added directly to your app using the Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this. |
federicobadini
force-pushed
the
upgrade-to-next-13
branch
from
cf7c5b1 to
5c3aec6
Compare
|
@federicobadini we merged #1755 to unblock the failing tests. |
federicobadini
force-pushed
the
upgrade-to-next-13
branch
from
05eba56 to
03de675
Compare
federicobadini
force-pushed
the
upgrade-to-next-13
branch
from
03de675 to
1bf2a2f
Compare
federicobadini
force-pushed
the
upgrade-to-next-13
branch
from
1bf2a2f to
55f9287
Compare
|
Everything appears to be working as before. QA 😺 |
| @@ -56,7 +56,7 @@ test.describe('Product Variant Tests', () => { | |||
| ).not.toBeVisible(); | |||
| }); | |||
|
|
|||
| test('Switch Variants and Add To Cart', async ({ | |||
There was a problem hiding this comment.
Is this meant to still be in here? This seems to skip the test that ensures variant switching and add-to-cart remain working.
There was a problem hiding this comment.
I see the pull description now. While I understand the motivation, I'm not sure I'm comfortable deploying a major change to our platform and also disabling our most important tests. Is there someplace we need to go badly enough that we feel it's worth the risk of driving without our seatbelts?
There was a problem hiding this comment.
Here are some of the reasons for upgrading we discussed in the past weeks:
Next.js 13 introduced app routing
This is a significant change compared to the page routing that was widely used in earlier versions of Next.js. The introduction of app routing has several consequences:
- It enables React Server Components, which provide a new way to improve performance - especially do reduce bundle related metrics.
- It opens up new opportunities for libraries to adapt and work in this new paradigm. For instance, we are evaluating how to localize content, and choosing a library that works well with this paradigm is crucial to avoid losing the benefits that React Server Components bring.
Overwriting headers and cookies in middlewares
Next.js 13 should also allow us to overwrite headers (and possibly cookies) in middlewares. This feature might be interesting for A/B testing.
MSW in monorepo
I had a chat with @ardelato in which he informed me that MSW is most likely not going to be included in the upcoming monorepo. As a result, those tests need to be restructured anyway.
Skipped test impact up to now
According to answers collected on slack, those tests seems not to have caught that many problems up to now (see here). I cannot be sure that every dev viewed this slack message though
To be clear, removing tests is not something that I like too, and I feel why you are in doubt.
However not going for an update also means tha we lose the opportunities above at least for some time.
There was a problem hiding this comment.
Building upon what @federicobadini has mentioned, @iFixit/qae has discussed the future of MSW in the monorepo and are leaning towards removing it. The original motive for integrating MSW was to mock server-side requests for fetching specific product types. Nonetheless, in the monorepo we can make use of our PHP testing extensions to set up these products in the test DB, thereby negating the necessity of MSW.
To address the lack of test coverage, we are working on expanding a script that would help populate the database for manual QA. https://github.com/iFixit/ifixit/issues/48312
|
There aren't any commit messages here, so it's hard to understand the purpose behind some of the changes. Can you explain some of the patterns? Like usage of dev_block 👍 on us being sure this is time-critical enough to warrant disabling all our tests. Neither the issue nor the pull give a justification for this being time critical. |
Co-authored-by: Mason McElvain <52104630+masonmcelvain@users.noreply.github.com>
|
I squashed the commits in order to avoid cluttering the PR with temporary changes that were made to get tests working or resolve compilation errors, which might have been confusing for the reviewers. In the future, if you prefer having the whole set of commits, I can certainly leave them 😊 Regarding the usage of |
I understand, back and forth changes or experimentation in commit history usually aren't important and often do lead to confusion, so squashing some of that makes sense. That said, having no commit messages, no pull description, and no issue description for a major version upgrade of the platform that also disables all the tests is way outside the norms. I'm not doubting that there are good reasons to plow ahead anyway, but I think it's a good idea to document the motivations and justifications in the issue or the pull request. Acknowledging that costs and risks were evaluated, and then recording the resulting decision is prudent and good practice and leaves a paper-trail to examine later. I vote for filling out the pull description with some of the information from your explanatory comment above. |
|
I have updated the PR description with many details describing what this PR is doing and why we are proposing to give precedence to this wrt waiting to solve MSW/test related issues 🙂 |
|
Is there any progress on accepting this PR? |
|
un_dev_block 👍 I'm OK with going forward on this. Still needs QA though. |
Conflicts:
frontend/templates/product-list/sections/ProductListChildrenSection.tsx
- Master remove the usage of NextLink here, so we don't need to set
`legacyBehavior`
|
Continues to work as expected. QA 😺 |
closes #1595
This pull request includes the following updates:
Next Image
The
next/future/imageis now the default image component as per Next.js 13 documentation. Some props were removed which required changes to both usages of thenext/imagecomponent and derived components likeResponsiveImage. A few style fixes were needed due to markup differences.Next Links
The new link component doesn't require a nested anchor tag anymore. Whenever possible, the new behavior was adopted. The
legacyBehaviourprop is available and has been used to reduce the number of changes in this PR for components that could've needed more intricate style/markup refactoring.Next Script
Scripts marked as
beforeInteractivewere moved to_document.tsxas suggested in Next.js documentation.Next Transpile Modules
Next.js now has its own transpile option, so the
next-transpile-modulespackage is no longer needed. For more information, check here.Sentry Edge Config
A Sentry edge config was added to address warnings about missing Sentry config files and to prepare for experiments with edge middlewares.
SSR Failure for Next Router
A workaround was added to prevent SSR failure in Next.js 13. Refer to these discussions for more information: issue #5650 and issue #5475.
MSW Issue
The MSW mocking utility is not fully compatible with Next 13, as discussed here. Since MSW will likely be replaced by other mocking strategies, problematic tests have been temporarily skipped.
The reasons we suggest prioritizing an upgrade over exploring new testing mechanisms are that Next.js 13 introduces a significant change with server components.
Delaying the adoption of this paradigm might be disadvantageous. During these weeks, several core aspects are being discussed, such as translations management, performance improvement, and A/B testing—all of which could benefit from the enhanced functionality offered by the new framework.
For additional details see here
Minor Updates
InstantSearchProvider.tsx.QA