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鈥檒l 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

Conversation

renrizzolo
Copy link
Contributor

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

  • always output css
    • fixes a bug with composed selector global css class not being applied in SSR with pages router
  • use 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.)
  • remove appDir config check

e2e / Fixtures

  • added next (v13) app router (@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 and features. I can add them all, but I figured the fixtures themselves are well covered.
  • added next dev and prod snapshot testing.
    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.
  • add build step for the next apps to the 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

  • Updated playwright to ^1.38.1. This allows to do the custom snapshotPathTemplate for screenshots, which also removes the need for the _autoSnapshotSuffix workaround.
  • bumped the port range due to chrome net::ERR_UNSAFE_PORT for 10xxx ports. I guess the newer playwright version has something to do with this popping up now?
  • add 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.

@changeset-bot
Copy link

changeset-bot bot commented Oct 23, 2023

馃 Changeset detected

Latest commit: 6798f25

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
@vanilla-extract/next-plugin Patch
@fixtures/features Patch
@fixtures/next-app-router Patch
@fixtures/next-pages-router Patch
@fixtures/recipes Patch
@fixtures/sprinkles Patch
@vanilla-extract-private/test-helpers Patch
@vanilla-extract-private/tests Patch

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

Copy link
Contributor

@askoufis askoufis left a 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 馃

packages/next-plugin/src/index.ts Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@renrizzolo renrizzolo force-pushed the fix-next13-pages-client-style-loading branch from aeb2580 to 71f18b4 Compare October 30, 2023 22:50
@askoufis
Copy link
Contributor

next-style-loader can mess up css order in development mode, but this is surely better than no css at all

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;
Copy link
Contributor Author

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

Comment on lines +23 to +25
"next": "npm:next@13.5.4",
"react": "npm:react@^18.2.0",
"react-dom": "npm:react-dom@^18.2.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@renrizzolo
Copy link
Contributor Author

I'm not entirely sure how to reproduce the next-style-loader development ordering issue to comment on it.
The only time i can see this happening is with hmr: change a style in a css.ts file, after saving it, it will be moved to then end of the style blocks in the document head.

@askoufis
Copy link
Contributor

I'm not entirely sure how to reproduce the next-style-loader development ordering issue to comment on it. The only time i can see this happening is with hmr: change a style in a css.ts file, after saving it, it will be moved to then end of the style blocks in the document head.

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.

@renrizzolo renrizzolo force-pushed the fix-next13-pages-client-style-loading branch from b52235d to 157cd2c Compare November 7, 2023 00:16
Comment on lines +70 to +108
## 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)
Copy link
Contributor Author

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.

@huypham50
Copy link

Following up on this issue.

@adrhumphreys
Copy link

I applied the fix in @vanilla-extract/next-plugin and it fixed the issue I was experiencing when navigating between pages. The CSS now is always applied and I don't need to refresh the page to load the CSS files.

It would be good awesome if we could look at getting that fix in as it would resolve #1152

@askoufis
Copy link
Contributor

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.

@renrizzolo renrizzolo force-pushed the fix-next13-pages-client-style-loading branch 4 times, most recently from 034e1c6 to 540a6b1 Compare February 14, 2024 09:00
@renrizzolo renrizzolo force-pushed the fix-next13-pages-client-style-loading branch from 540a6b1 to 6798f25 Compare February 14, 2024 09:20
@askoufis
Copy link
Contributor

@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:

  1. Updating the playwright version and tweaking the config, resulting in those snapshot file renames
  2. Adding Next.js integration tests
  3. Fixing Next.js pages router styles

Hoping it's not too hard to pick things apart. Happy to help out if you need.

@huypham50
Copy link

@adrhumphreys Just bumped all of my VE dependencies to latest and the css is still getting generated incorrectly locally. Had to downgraded @vanilla-extract/next-plugin back to 2.1.3. Looking forward to this fix!

@askoufis
Copy link
Contributor

@renrizzolo Should we close this in favour of #1365?

@renrizzolo renrizzolo closed this Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants