-
Notifications
You must be signed in to change notification settings - Fork 292
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 next pages router style loading, add next snapshot tests #1203
Fix next pages router style loading, add next snapshot tests #1203
Conversation
🦋 Changeset detectedLatest commit: 6798f25 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
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.
Really appreciate the effort on this. I've got just a few comments, we'll see how things go on CI 🤞
aeb2580
to
71f18b4
Compare
If we're going to add this functionality back in, it should probably be documented as a caveat/gotcha in the next integration docs. |
true | ||
: // There is no appDir, do not output css on server build | ||
!isServer; | ||
const outputCss = true; |
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.
for reference, if outputCss
is false on the server (using pages router), the class created by the globalStyle below won't be applied to the ssr'd html
export const styleCompositionInSelector = style([
style({ color: 'white' }),
style({ backgroundColor: 'black' }),
]);
globalStyle(`body ${styleCompositionInSelector}`, {
fontSize: '24px',
});
it will work if the selector looks like this
export const styleCompositionInSelector = style([
{},
style({ color: 'white' }),
style({ backgroundColor: 'black' }),
]);
I think this is the same issue as #1133
"next": "npm:next@13.5.4", | ||
"react": "npm:react@^18.2.0", | ||
"react-dom": "npm:react-dom@^18.2.0" |
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'm not entirely sure how to reproduce the |
Ah ok. I suppose that's almost what you'd want for HMR anyway. You'd want the most recent version of a style to be used, so if there's a conflict the newly inserted style should win. |
b52235d
to
157cd2c
Compare
## CSS load order issues | ||
|
||
There are some known issues with Next.js css load order and/or duplicate css. | ||
|
||
- When client-side navigating, new css is injected into the head, but unused css isn't removed. This means if route B has a component that applies some global css to a component also in route A, upon returning to route A, those styles from route B will still be applied. | ||
- When using App router, css can be duplicated which could cause unexpected overrides. | ||
|
||
[next.js #51030](https://github.com/vercel/next.js/issues/51030) | ||
[next.js #40080](https://github.com/vercel/next.js/issues/40080) |
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.
@askoufis this is how I see it from my testing, but maybe I'm missing something.
There are quite a few issues in the next.js repo regarding css loading order etc., although it doesn't seem as bad with vanilla extract as it is with css modules for example.
Here's a repro demonstrating the lingering css overrides. This seems to happen in every mode/router aside from pages router in production.
https://codesandbox.io/p/sandbox/next-51030-forked-phqdsr
To some degree it's kind of expected behavior? I guess we need to keep tabs on any future improvements on the next side.
a8f807c
to
174e112
Compare
174e112
to
72c7a66
Compare
Following up on this issue. |
I applied the fix in It would be good awesome if we could look at getting that fix in as it would resolve #1152 |
Sorry for not following up on this. I'll try and get to this soon. Feel free to fix the conflicts if you get a chance @renrizzolo, otherwise I'll take a crack and try get this through. |
034e1c6
to
540a6b1
Compare
540a6b1
to
6798f25
Compare
@renrizzolo I'm liking where the PR is at, but it is quite big, so IMO it's worth de-risking these changes a bit by splitting things up into separate PRs. There's probably 3 separate PRs here:
Hoping it's not too hard to pick things apart. Happy to help out if you need. |
@adrhumphreys Just bumped all of my VE dependencies to |
@renrizzolo Should we close this in favour of #1365? |
I thought it would be a good idea to have some e2e tests for Next.js, seeing as there have been a few issues, particularly with the introduction of the App Router / keeping backwards compatibility with next 12.
There was a lot of trial and error to get all this working, so if there's anything that looks odd I should have an explanation 😄
Next Plugin
next-style-loader
for pages router in development (as per original comment, next-style-loader can mess up css order in development mode, but this is surely better than no css at all.)e2e / Fixtures
@fixtures/next-app-router
) and next (v12) pages router (@fixtures/next-pages-router
).These apps themselves import other fixtures, and serve them at
/[fixture]
.NOTE: I only added
recipes
,sprinkles
andfeatures
. I can add them all, but I figured the fixtures themselves are well covered.These need to be separate, and serial, because next can't really handle serving dev/prod at the same time.
In order to be able to assert the screenshot from next-*/[fixture] matches the screenshot of [fixture], I had to move all screenshots to a single folder. I left the CSS in the existing test folders to at least keep them separate.
test:playwright
command. If this is annoying for development we could add a separate CI version of this command that is called only in the validate workflow.Other
snapshotPathTemplate
for screenshots, which also removes the need for the_autoSnapshotSuffix
workaround.net::ERR_UNSAFE_PORT
for 10xxx ports. I guess the newer playwright version has something to do with this popping up now?dedupe-peer-dependents
to.npmrc
. This is the default in pnpm 8, and was needed so that multiple versions of next 12 wouldn't be installed and used by the next plugin and next 12 fixture app respectively.