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.

Sign up for GitHub

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

Jump to bottom

fix(nextjs-plugin): properly handles css loader #1105

Merged
merged 5 commits into from
Jul 24, 2023
Merged

fix(nextjs-plugin): properly handles css loader #1105

merged 5 commits into from
Jul 24, 2023

Conversation

SukkaW
Copy link
Contributor

@SukkaW SukkaW commented May 30, 2023 •
edited

Closes #1101.
Closes #1131.

Changes:

  • Always choose css-loader/MiniCssExtractPlugin.loader instead of getClientStyle from Next.js (which will opt-in next-style-loader during development mode in the pages directory):
    • next-style-loader will mess up CSS order which is crucial for vanilla-extract
    • Next.js App Router always uses css-loader and does not use next-style-loader at all anyway.
  • Add webpack to devDependencies
    • To add the type definition for the loaders array.
  • Update peerDependencies.next to >= 12.1.7
    • Next.js 12.1.7 is the first version that has MiniCssExtractPlugin's HMR fixed.
  • Change outputCss logic:
    • Always output CSS when there is the app directory since Next.js App Router needs to collect CSS from React Server Components.
    • When there is only a pages directory, output CSS only on the client build.
    • Uses Next.js' internal findPagesDir method to determine if the app directory exists.
  • Make sure MiniCssExtractPlugin exists in the webpack config.

@changeset-bot
Copy link

changeset-bot bot commented May 30, 2023 •
edited

🦋 Changeset detected

Latest commit: d1853f5

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

This PR includes changesets to release 1 package
Name Type
@vanilla-extract/next-plugin Minor

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

@SukkaW SukkaW changed the title fix(nextjs-plugin)L proper handle css loader fix(nextjs-plugin): proper handle css loader May 30, 2023
@SukkaW SukkaW changed the title fix(nextjs-plugin): proper handle css loader fix(nextjs-plugin): properly handles css loader May 30, 2023
@mattcompiles
Copy link
Contributor

Thanks for the PR @SukkaW

The changes you're making are quite extensive, I don't feel knowledgeable enough about Next to validate it.

@SuttonJack @shuding Would really appreciate your thoughts on this?

shuding
shuding approved these changes Jul 6, 2023
View reviewed changes
Copy link
Contributor

@shuding shuding left a comment

Choose a reason for hiding this comment

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

Thanks, these look good to me

@SukkaW SukkaW requested a review from tavvy July 21, 2023 13:11
@SukkaW
Copy link
Contributor Author

SukkaW commented Jul 23, 2023

@mattcompiles Just a friendly ping. Would you like to review my PR?

mattcompiles
mattcompiles reviewed Jul 24, 2023
View reviewed changes
Copy link
Contributor

@mattcompiles mattcompiles left a comment

Choose a reason for hiding this comment

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

@SukkaW Sorry mate. We really appreciate the PR, just struggling with balancing regular life ATM. I honestly don't feel knowledgable enough about Next internals to review this, however I'm comfortable to merge it given the feedback from the others.

Currently the branch is out of date and failing some lint steps in CI which you can run with pnpm lint. I'll approve and merge as as soon as that's green. Just ping me again once you've pushed it up.

@SukkaW
Copy link
Contributor Author

SukkaW commented Jul 24, 2023 •
edited

@mattcompiles

I have run the actions in my own repo, and they are all green now.

SukkaW#1

image

@mattcompiles mattcompiles merged commit 9541d79 into vanilla-extract-css:master Jul 24, 2023
4 checks passed
@vanilla-extract-ci vanilla-extract-ci mentioned this pull request Jul 24, 2023
Merged
This was referenced Jul 25, 2023
Closed
Closed
@janbiasi janbiasi mentioned this pull request Jul 25, 2023
Open
2 tasks
@janbiasi
Copy link

@SukkaW unfortunately this did not fix #1131

@arnarleifs
Copy link

I noticed you talked about version 2.2.0 but the latest version is 2.2.1, which was released an hour ago. Also there is an update to the @vanilla-extract/recipes package. Can you confirm it still exists after updating both packages?

@SukkaW SukkaW deleted the nextjs-plugin-fix branch July 30, 2023 10:01
@MarcMcIntosh MarcMcIntosh mentioned this pull request Aug 2, 2023
Merged
@edenstrom edenstrom mentioned this pull request Aug 24, 2023
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shuding shuding shuding approved these changes

@mattcompiles mattcompiles mattcompiles approved these changes

@SuttonJack SuttonJack SuttonJack approved these changes

@tavvy tavvy Awaiting requested review from tavvy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
7 participants
@SukkaW @mattcompiles @janbiasi @arnarleifs @tavvy @shuding @SuttonJack