-
Notifications
You must be signed in to change notification settings - Fork 651
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
feat(plugins): add unocss plugin (WIP) #1303
Conversation
Hey @marvinhagemeister! From your comment in the August 2023 iteration plan issue (didn't want to pollute that conversation), I'm curious to ask what's your stance on That being said, I haven't had the time to continue with the to-do items in this PR, but I've been using this plugin extensively and can confirm "it just works". Eager to hear your opinon, I wonder why most people still try to work with |
Super excited to have UnoCSS in Fresh. I have some decently sized apps to test this on for regressions (which I will do later on in the review process for this PR). This is much more "fresh"-y in nature and kind of avoids a whole bunch of twind-related issues. I haven't had the chance to look at code yet but huge +1 |
I'm all for adding support for |
Any chance you could push this forward @miguelrk? I'd love to pick this up if not. |
Thanks for the interest and reminder @lino-levan ! I added some basic docs based on the existing In any case, do you think we should be defaulting to using the I'm leaning towards removing the defaults, since the defaults are pretty simple an can be manually configured inline in a single line: import unocssPlugin from "$fresh/plugins/unocss.ts";
import presetUno from "@unocss/preset-uno";
await start(manifest, { plugins: [unocssPlugin({ presets: [presetUno()] })] });``` and also very easily imported from another file: import unocssPlugin from "$fresh/plugins/unocss.ts";
import unocssConfig from "./uno.config.ts";
await start(manifest, { plugins: [unocssPlugin(unocssConfig)] });``` What do you think @lino-levan ? |
Hey @miguelrk! I can't seem to get the VSCode extension to work, so my conclusion is that it likely doesn't matter. I think we should have a default config to make migration easy, but we should encourage people to use their own config by making a |
Hmm now that you mention it @lino-levan, the |
Yeah, we should look into that. I'm sure they'd be happy to accept a PR.
Sure. Let's do that instead. Having no default doesn't seem unreasonable. |
Great! I've done that in b397d6f and updated the docs. Something else that comes to mind? |
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.
Outside of tests, this is looking pretty close to ready to me.
Is there any reason you can't write a fixture test like we have for |
Raised #1701 |
unocss/unocss#3040 will make Fresh and UnoCSS happy together on VSCode. |
Huge thanks for this @zhmushan ! From our conversation earlier:
I still don't get why adding support for |
Because we cannot be certain that uno.config is specifically designed for Fresh, separating its original logic from Deno's special handling requires some additional work (although it may not necessarily be complex, there might be boundary conditions to consider). On the other hand, for fresh.config, we can confidently determine that it can be handled use Deno, and the UnoCSS plugin has a designated position for handling framework configuration files. Therefore, only minimal effort is needed to achieve quick adaptation. |
@zhmushan I see... I'm still uncomfortable with removing support for Alternative 1 If I understand correctly, what prevents us from using If the above is true, we could import from // unocss/packages/vscode/deno/fresh.ts
- const { default: freshConfig } = await import(Deno.args[0])
- const unoConfig = freshConfig.plugins.filter(it => it.name === 'unocss')
+ const { default: unoConfig } = await import(Deno.args[0])
console.log(JSON.stringify(unoConfig)) Alternative 2 A simpler workaround (for now) could be declaring the unocss config in // uno.config.ts
export default defineConfig({...})
// fresh.config.ts
export { default as unoConfig } from "./uno.config.ts"
export default defineConfig({...})
// unocss/packages/vscode/deno/fresh.ts
const { unoConfig } = await import(Deno.args[0])
const unoConfig = freshConfig.plugins.filter(it => it.name === 'unocss')
console.log(JSON.stringify(unoConfig)) |
My PR does not mean permanently giving up on adapting Ideally, The prerequisites for adapting Deno are as follows:
|
For sending the config to the runtime, could we maybe avoid esbuild and the need for a const configStr = JSON.stringify(config, (key, value) => (typeof value === "function")? value.toString() : value); I believe that would allow standard Uno config files without the extra |
I've gone off this idea because of the possibility of functions including closures or referencing module-level variables, which would not be serialised. Instead, I think a better alternative to |
- Import config from uno.config.ts if no config object is explicitly provided - Always use uno.config.ts as the import source for the browser runtime config - This avoids the complexity of selfURL, allowing the plugin to use standard Uno config files
It is the same as found in the unocss package, but that can not currently be safely imported in Deno (due to Node-specific code in the icons preset).
UnoCSS plugin use uno.config.ts
Make UnoCSS plugin function synchronous
Hey @marvinhagemeister ! I see you've been working on build-hooks in relation maybe to #1700 and #1701 (for which there's also @adamgreg's PR #1769). This might just be what we were missing to better integrate UnoCSS to the plugin. However, there will still be some scenarios where shipping the unocss runtime to the client is inevitable, for which it might be best that this plugin provides the following 3 independent options (proposed by @adamgreg):
Other than that, I've merged @adamgreg's latest changes enforcing the use of the standard |
I've found a problem with running Any generation of CSS during SSR should be done via a Preact hook, like the twind plugin does. |
I've made a PR to improve SSR into this branch: https://github.com/miguelrk/fresh/pull/4 |
UnoCSS: Use Preact hook for SSR
Based on the latest discussion of this with @marvinhagemeister on the Discord channel, it sounds like we should generally be targeting AOT usage, not JIT. I think we just need a single JIT/AOT option:
@marvinhagemeister @miguelrk thoughts? |
Transformer support is perhaps not essential... however... I believe the only way to support transformers would be by plugging into the Preact render, JIT-style. Before running class extraction over the source code, we could run transformers to make sure that all the necessary classes are generated. However, the HTML/DOM rendered JIT by Preact would itself still need to be transformed in order to actually use those generated classes. A disabled-by-default plugin option could enable transformer support, with the added render performance cost. Maybe at some point UnoCSS will add an extractor which works with un-transformed variant group classes, as an alternative to the variant group transformer. That seems to me the only really compelling use for transformers at the moment. |
@adamgreg Oh does UnoCSS only do variants through transpilation? |
Afraid so: https://unocss.dev/transformers/variant-group. It seems to me like it should be possible to extract directly, but maybe the parentheses or spaces would be a problem for class names |
I've just checked, and WindiCSS also transpiles the HTML to make variant groups work. I think it's inevitable - there's no way to escape spaces in CSS class names. The parentheses aren't a problem though, and it would be possible to implement a variant 🙄 of this feature in a pure extractor, using a different delimiter instead of spaces. I've experimented a little here: https://jsfiddle.net/kypjLdc8/ . I think commas are very understandable. It's more of a UnoCSS feature request than a Fresh one though. In any case, I think it would be good if the plugin allowed opt-in to transformers by patching into Preact. AOT support is the priority though. |
As discussed with @adamgreg, closing this in favor of the following with full support for AOT builds: Linking these other relevant PRs as well: |
TLDR:
unocss
is an instant on-demand atomic CSS engine and a joy to use. I believe it can (and should) replacetwind
as the default atomic CSS engine for fresh.Context
[email protected]
introduced therenderAsync
method, which allows calling the asynchronousuno.generate()
method as first proposed by @Hladikes (thanks!) but without the workaround. I've tested it for routes, components and islands, and it's working for all (tests are pending).Checklist
This PR is a WIP... I will continue tomorrow with the following:
Related Issues
#728
#290