Conversation
🦋 Changeset detectedLatest commit: 4ff3498 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 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 |
|
@mattcompiles @askoufis Can anyone take a look at this? This is a pretty important feature for next.js users. Or should we consider the project dead? Last PR was merged in early August, basically no activity from maintainers since then. Not meaning to bash anyone, I know it's thankless work maintaining open source software, but it would be nice to know what we can expect from this project. Thanks in advance! |
A PR was merged in September. Reviews take time and ongoing work is often slow. All current maintainers have professional commitments and/or families. I try to look at PRs when I have spare time/motivation, which is often quite sporadic. I'm currently on holiday, but I intend to make some headway on open PRs and repo maintenance in when I get back in early November. |
|
Thanks for your reply! I was a bit too dramatic, sorry about that. Enjoy your holiday, and let me know if I can help with anything to get this PR going. |
|
I see there have been some recent commits pushed up to this PR. Are there any bits that you need help with that could be provided to speed up getting this merged in? Appreciate that it's been a holiday period, but a lot of apps have performance issues with Next which has various potential solutions but the main recommendation seems to boil down to "just use Turbopack" vercel/next.js#48748. Obviously, the lack of support for Turbopack with Vanilla is effectively blocking us from getting those gains, which would be greatly appreciated ahead of Black Friday. It would be great if there were a list of tasks that I or people within my company could help out with to speed this up, whether that's commits or just QAing. Cheers and thanks for the work on this so far! |
This reverts commit 568bd26.
|
If you're interested in trying this out, I've been using a You can install that temporary build with a link: Although, you probably want to pin to a specific commit instead of the branch for the sake of stability: Feedback or issues appreciated! I haven't seen many stats yet on speed or size of builds (compared to webpack) so if you have those I'd love to see some details. The biggest issue with this at the moment is I'm gonna do a quick cleanup pass this week to prep this for merge, it's basically ready at this point but is still a bit messy while I'm experimenting with some stuff. TLDR:
|
|
Hmmm, strange, I followed your steps and got this error That seemed to point at this specific line in the dist It's probably worth saying that we don't have a pages dir at all, we're fully on app dir. To be fair as well, I'm not sure that this would work for us anyway as we make heavy use of When you say it's basically ready, do you mean basically ready minus support for next modules due to dependency on things turbopack isn't yet providing to help you preprocess styles? If you have a solution to the above error, more than happy to give it a whirl |
|
The error looks like resolution error in the webpack plugin setup. Should've seen that coming. As far as next/image support, you're free to style next images as this doesn't route through the plugin: The specific scenario that doesn't work is importing an image into a This is unlikely to change until we get better tools from turbopack. For The specific scenario that doesn't work is extending font classNames in your styles: This is also unlikely to change until we get better tools from turbopack. TLDR using next modules is fine, but using next modules in your styles themselves is likely to fail unless I've specifically worked around it. |
For me is fine, thanks, I also had next font issues with webpack so I don't mind this approach, thank you very much for your work, I'll try this ASAP |
|
Hello, when is this planned on being approved and release? This would be so good to get in soon, tested it out with this branch and it seemed to be working great. |
|
I've also tested these changes in a relatively complex Turbopack Next project and everything worked flawlessly. |
I have not but am invested in this thread :-) how can I persuade a merge? |
|
Also very invested in this thread! |
|
Along with many others, this PR is a blocker for us moving fully onto Next with Turbopack (we’re otherwise on Webpack due to lack of support here). Could a maintainer share a rough status or timeline for review/merge, or point to anything still needed from contributors? We’re happy to help with rebases, fixes, or testing to get this ready. Thanks |
|
It's been almost six months. I think it's safe to say this is not going to get merged. It might be a good idea to see if we can turn this PR into a plugin, or "just" fork? |
|
Maybe @mattcompiles and @askoufis should promote @RJWadley or some other contributors to be Maintainers of Vanilla Extract if they don't have the time |
|
I started reviewing this over the weekend. It's a large PR so it will take some time. |
|
This might be an edge case, but I am hitting a failure when For example: //next.config.ts
export const config: NextConfig = {
outputFileTracingRoot: path.join(__dirname, '../../'),
};With this configuration, Turbopack log the following error: Here is the complete log Here is a minimal reproduction on the try/outputFileTracingRoot branch: Notes:
|
|
+1 on this, would be a massive win to have this in. currently blocked by this, and my company would love to use vanilla extract out the box with our turbopack next app |
@zecka I wasn't able to reproduce the issue on this repository when I cloned it. I would presume the issue is caused by the way things are linked on your filesystem. Just guessing here, but one potential issue that I have seen before is that you may need to configure Since you're using |
Implements Turbopack compatibility based on PR vanilla-extract-css#1639 by @RJWadley: - packages/compiler: add `splitCssPerRule` option, `invalidateAllModules()` method, and update `cssImportSpecifier` to accept CSS content + return Promise<string> - packages/css: add `vanilla.virtual.css` noop file for virtual CSS imports, update package.json exports and files list - packages/next-plugin: auto-detect Next >= 16 and configure Turbopack rules; add `turbopackMode` ('auto'|'on'|'off') and `turbopackGlob` options; add semver + @vanilla-extract/turbopack-plugin dependencies - packages/turbopack-plugin: new package — Turbopack webpack loader that compiles *.css.ts files via @vanilla-extract/compiler, encodes CSS into virtual query-param imports on vanilla.virtual.css, and handles next/font stubs via SWC transform https://claude.ai/code/session_0145VazPBZC9Q6nKK8gfjrXf
You’re right, it’s probably related to my local system configuration. |
askoufis
left a comment
There was a problem hiding this comment.
I've got a few questions and suggestions but overall this looks good. I'll do another review once the feedback has been addressed and I get more time to run the fixtures locally.
| "test:playwright": "pnpm test:build-next && playwright test", | ||
| "test:build-next": "node scripts/copy-next-plugin.ts && pnpm --filter=@fixtures/next-* clean-build", | ||
| "test:playwright": "pnpm test:clean-next && pnpm test:build-next && playwright test", | ||
| "test:clean-next": "pnpm --filter=@fixtures/next-* run '/^clean.*/'", |
There was a problem hiding this comment.
I'm a bit confused about test:clean-next because it looks like you've removed the clean scripts from the next fixtures. Should they have been removed?
There was a problem hiding this comment.
the cleaning is all still in there, I just removed the clean-build shortcut that calls clean + build.
this is just because we're testing webpack + turbopack and I wanted them to build in parallel, and it seemed like the easiest way at the time.
I would rather remove the cleaning entirely, as it shouldn't be needed, but thought that would be best for a future PR if at all.
| @@ -0,0 +1,13 @@ | |||
| import { NextConfig } from 'next'; | |||
| import { createVanillaExtractPlugin } from './next-plugin/dist/vanilla-extract-next-plugin.cjs.js'; | |||
There was a problem hiding this comment.
Should this import be using the package specifier rather than relative path?
| import { createVanillaExtractPlugin } from './next-plugin/dist/vanilla-extract-next-plugin.cjs.js'; | |
| import { createVanillaExtractPlugin } from '@vanilla-extract/next-plugin'; |
There was a problem hiding this comment.
that would be better IMO, but neither of the existing next fixtures do, and we'll experience type errors if we try. see scripts/copy-next-plugin.ts for a bit more context.
this is another thing I'd like to clean up soon, but opted to just match the status quo for the first revision. happy to take another look if you want it in this PR.
| "vite": "^5.0.0 || ^6.0.0 || ^7.0.0" | ||
| }, | ||
| "peerDependencies": { | ||
| "next": ">=12.1.7" |
There was a problem hiding this comment.
Given the docs say only next>=16 supports the turbopack plugin, should this version range reflect that too, or is this purely so package managers don't complain when a user that doesn't want to use turbopack installs @vanilla-extract/next-plugin?
There was a problem hiding this comment.
this is purely so that package managers don't complain. users don't install turbopack-plugin directly, it's installed by the next plugin. so any users on old next version would see peer errors.
Turbopack will only configure itself if next is >= 16. The user could still attempt to manually configure turbopack on next 15, but it would always crash if they did. It would be trivial to warn in this case, which I think is a good idea.
edit: added
packages/compiler/src/compiler.ts
Outdated
| /** | ||
| * When true, generates one CSS import per rule instead of one per file. | ||
| * This can help bundlers like Turbopack deduplicate shared CSS more effectively. | ||
| * | ||
| * @default false | ||
| */ | ||
| splitCssPerRule?: boolean; |
There was a problem hiding this comment.
I'm a bit confused about this. If you're already sharing the same VE style in multiple places, isn't there no need to deduplicate anything?
There was a problem hiding this comment.
I can't confirm how webpack or any other integration works, but before adding this we were seeing massive amounts of duplication across reused files. For example, every file that used our color library would re-bundle the entire color library (and it would be included hundreds of times)
I added a test that catches some common duplication issues, and splitting each rule out per import was the easiest way to deduplicate them all.
| type PluginOptions = ConstructorParameters<typeof VanillaExtractPlugin>[0] & { | ||
| turbopackGlob?: string[]; | ||
| /** | ||
| * controls whether we attempt to configure turbopack | ||
| * auto: enable only when Next >= 16.0.0 | ||
| * on: force enable regardless of detected Next version | ||
| * off: never configure turbopack, webpack only | ||
| */ | ||
| turbopackMode?: 'auto' | 'on' | 'off'; | ||
| }; |
There was a problem hiding this comment.
I'd prefer these options to be nested under an unstable_turbopack property for 2 reasons:
- I'm not confident that this integration won't require breaking changes in the near future
- Grouping
turbopack-related options together seems a bit neater than prefixing everything withturbopack
There was a problem hiding this comment.
this is actually great idea, I might also do this to the compiler option I added to reset the cache (since it's only there to work around a bug in the compiler)
edit: done. also renamed the two experimental compiler apis.
There was a problem hiding this comment.
do you also want turbopack support to disable to "off" instead of "auto"? auto mode configures turbopack if we're on next >=16
| }, | ||
| "exports": { | ||
| "./package.json": "./package.json", | ||
| "./vanilla.virtual.css?*": "./vanilla.virtual.css?*", |
There was a problem hiding this comment.
Is the ?* necessary here? AFAIK query parameters should be preserved during module resolution, and they don't need to be specified in export conditions.
| "./vanilla.virtual.css?*": "./vanilla.virtual.css?*", | |
| "./vanilla.virtual.css": "./vanilla.virtual.css", |
There was a problem hiding this comment.
I did not originally include it, but iirc I added it because turbopack does require it. will double check.
There was a problem hiding this comment.
confirmed. turbopack does not strip query strings before matching against the exports field. without the wildcard, every CSS import will fail.
| return `@vanilla-extract/css/vanilla.virtual.css?ve-css=${encodeURIComponent( | ||
| await serializeCss(css), | ||
| )}`; |
There was a problem hiding this comment.
Can the vanilla.virtual.css file live inside the turbopack plugin package instead of the css package? I'd rather it be associated with the plugin it's relevant to, rather than being included in the css package.
There was a problem hiding this comment.
turbopack must be able to resolve the import from the source file itself - so if you're compiling styles across packages in a monorepo (like how, for example, our test fixtures do) turbopack would not be able to resolve @vanilla-extract/turbopack-plugin. Turbopack searches for that module from the importing file, so the only package that is guaranteed to exist is @vanilla-extract/css. Turbopack refused to resolve relative imports here, and it doesn't support importing the absolute path.
Originally I used data imports here but those are not deduplicated by turbopack.
Resolution and compilation across package boundaries is something I'd like to revisit at some point, as every integration seems to handle this slightly differently.
Co-authored-by: Adam Skoufis <askoufis@users.noreply.github.com>
Co-authored-by: Adam Skoufis <askoufis@users.noreply.github.com>
Co-authored-by: Adam Skoufis <askoufis@users.noreply.github.com>
|
@askoufis thanks for the review! addressed feedback and replied to comments, I will leave resolving them up to you at your satisfaction. |
adds turbopack support to
@vanilla-extract/next-pluginturbopack doesn't have any sort of external API yet, so in order to evaluate the contents of our
*.css.tsfiles we need to compile it separately using@vanilla-extract/compiler. To avoid import resolution mismatches, we defer module resolution to turbopack via its webpack-compatible api.next/fontdoesn't work out of the box because turbopack has custom logic for it, so we transform any modules that use it in a css context to contain static references to the generated font family, style, and weight.next/imagegets a similar treatment. most other next modules won't work incss.tsfiles for now.fixes #1367