feat(extractor-arbitrary-variants)!: make arbitrary extractor standalone #2364

antfu · 2023-03-17T14:32:44Z

I feel the current arbitrary variant support is a bit polluting the core too much. Ideally, the core should be agnostic. Also, I think arbitrary variants might not be needed for everyone, so making it a separate PR so ppl could opt-in when needed, and ppl don't use it do not need to pay the performance cost.

/cc @chu121su12 @sibbng let me know what you think, thanks!

netlify · 2023-03-17T14:32:51Z

✅ Deploy Preview for unocss ready!

Name	Link
🔨 Latest commit	`6ff5bac`
🔍 Latest deploy log	https://app.netlify.com/sites/unocss/deploys/643276726100ce00089e7deb
😎 Deploy Preview	https://deploy-preview-2364--unocss.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

sibbng · 2023-03-17T17:52:22Z

test/assets/preset-mini-targets.ts

We still need these tests. They are not tested against extractors because of being provided as an array. They are only tested against rules and variants.

https://github.com/unocss/unocss/blob/f27b1fd3a1557808f35281c5f4bf601ca7422904/packages/core/src/generator/index.ts#L131-L135C16

We should concat presetMiniTargets and presetArbitraryTargets in tests, and also join them into a string to be sure generated CSS is the same as the array.

test/pos.test.ts

sibbng · 2023-03-17T18:07:03Z

test/assets/preset-wind-targets.ts

@@ -394,7 +394,6 @@ export const presetWindTargets: string[] = [

  // variants experimental
  '@hover-text-red',
-  '@hover:[[open]_&]:text-blue',


We should add a separate test for this in preset-wind.test.ts.

It has been moved to

unocss/test/assets/preset-arbitrary-target.ts

Lines 16 to 17 in 350fd1a

// variants experimental

'@hover:[[open]_&]:text-blue',

sibbng · 2023-03-17T18:26:07Z

packages/preset-arbitrary/src/split-arbitrary.ts

+  code
+    .split(defaultSplitRE)
+    .forEach((match) => {
+      if (isValidSelector(match) && !arbitraryPropertyCandidateRE.test(match))
+        result.add(match)
+    })


This probably will not work as it was working previously in split.ts. The purpose of these lines is to reduce some false positives captured by defaultSplitRE. We remove those false positives on line 12.

If I remember correctly, this is capturing false positives like that:

hover="[&>span]:text-white"

Which is excluded from the tests currently.

split.ts will capture [&>span]:text-white and presetAttributify will capture hover="[&>span]:text-white" and unocss will generate unused [&>span]:text-white.

If this is a thing, we can't merge this PR without resolving this regression.

Maybe we should provide code variable as a MagicString instance and remove the matched utilities from the code. That means this extractor should run before the default splitter (split.ts).

Another solution is would be removing the default splitter from the extractors list since this extractor already does its job. This is not an ideal solution and we don't have access to uno config on here as of now.

To me, it's actually fine as a little redundant in UnoCSS is invertible, we the rules merge it shouldn't have much impact.

Meanwhile, when not using preset-arbitrary, it shouldn't generate such rule. While if using preset-arbitrary with atrributify, it replaces the default split function, which makes it identical to the previous behavior.

Having a "preprocessor" for extractors is a great idea and I have been thinking it for a while as well. As currently the pug and svelte extractor are coupled with the default extractor.

I will see how we can do it in another PR.

sibbng

Can we have some benchmarking tests to see performance cost before/after?

antfu · 2023-03-18T11:23:08Z

Can we have some benchmarking tests to see performance cost before/after?

While actual performance is not my concern here. What I am concerned about is the "agnostic" part. The core should not be aware of how the utils are defined in presets, and split.ts should provide a minimal and transparent implementation for the possibility of other presets to use.

antfu · 2023-03-18T11:32:35Z

Ran nr bench against this branch and the main, the delta cost of unocss is 140.01 ms vs 150.79 ms, I think (10ms, 7%) is not that significant but already quite noticeable. I imagine the different could get higher when we have more files in the project. But again, I consider performance not a concern here.

sibbng · 2023-03-18T17:16:54Z

While actual performance is not my concern here. What I am concerned about is the "agnostic" part. The core should not be aware of how the utils are defined in presets, and split.ts should provide a minimal and transparent implementation for the possibility of other presets to use.

Recently added configResolved almost no-op when not using presetAttributify({ strict: false }). Because we don't include extractorSplit when an extractor is already defined.

unocss/packages/core/src/config.ts

Lines 59 to 62 in 8554bf4

    
           const extractors = mergePresets('extractors') 
        
           if (!extractors.length) 
        
             extractors.push(extractorSplit) 
        
           extractors.sort((a, b) => (a.order || 0) - (b.order || 0))

And this already meets the "agnostic" part. So, instead of moving arbitrary extractor to a separate package, we should move it to preset-mini. This way, the core extractor can stay agnostic, and our preset can still process the possible candidates that they already target.

Ran nr bench against this branch and the main, the delta cost of unocss is 140.01 ms vs 150.79 ms, I think (10ms, 7%) is not that significant but already quite noticeable. I imagine the different could get higher when we have more files in the project. But again, I consider performance not a concern here.

My test results: (windi, tailwind disabled, 50x)

main branch:

3/18/2023, 5:36:23 PM
1656 utilities | x50 runs (75% build time)

none                             36.26 ms / delta.      0.00 ms 
unocss       v0.50.6            401.40 ms / delta.    365.14 ms (x1.00)

feat/preset-arbitrary branch:

3/18/2023, 5:29:52 PM
1656 utilities | x50 runs (75% build time)

none                             37.53 ms / delta.      0.00 ms 
unocss       v0.50.6            384.21 ms / delta.    346.68 ms (x1.00)

feat/preset-arbitrary is 5.34459% faster.

OK, there are some performance wins here. But I don't think it's worth introducing a breaking change. Since performance is not a main concern here instead of introducing a breaking change, we should make it customizable as part of the presets, and people who don't need it can disable it. And the core extractor will stay agnostic.

I see no point in moving this extractor to a separate package and making it default disabled by default.

Continuing the above discussion here:

To me, it's actually fine as a little redundant in UnoCSS is invertible, we the rules merge it shouldn't have much impact.

Merging rules will only work if we used those false positives in class attribute. IMO even on a big size project, this is very unlikely to happen. And the real issue is not even that. The real issue is those false positives are highlighted on VS Code. pos.test.ts still have false positives. My guess is that you need to update shared-common/index.ts too but I'm not sure.

Having a "preprocessor" for extractors is a great idea and I have been thinking it for a while as well. As currently the pug and svelte extractor are coupled with the default extractor.

I was thinking of more unified pre/post processor API. IMO we should deprecate all preprocess/postprocess/configResolved options and have a hook option with a bunch of hooks that can take control on generating process, e.g.

pre:generate(hookSpecificContext, ctx)
If a hook returns false its internal step will be skipped and post hook will be called.
post:generate(hookSpecificContext, ctx)
pre:extract(hookSpecificContext, ctx)
post:extract(hookSpecificContext, ctx)
pre:parseToken(hookSpecificContext, ctx)
post:parseToken(hookSpecificContext, ctx)
Could be useful to modify declarations, for example: rem to px), We can also extract our caching mechanism to this hook.
pre:stringify(hookSpecificContext, ctx)
Similar to the current postprocess hook, but you have access to whole list of utilities. that means you can add/duplicate some utilities.
post:stringify(hookSpecificContext, ctx)
pre:layer(hookSpecificContext, ctx)
post:layer(hookSpecificContext, ctx)

sibbng · 2023-03-18T17:22:01Z

test/pos.test.ts

+            46,
+            65,
+            "[&>span]:text-white",
+          ],


As I mentioned in my last comment, this shouldn't be here.

sibbng · 2023-03-18T17:22:14Z

test/pos.test.ts

+          [
+            13,
+            14,
+            "b",
+          ],


As I mentioned in my last comment, this shouldn't be here.

sibbng · 2023-03-18T18:50:58Z

packages/preset-arbitrary/src/index.ts

+    variants: [
+      variantVariables,
+    ],


IMO variantVariables should stay in its original place in preset-mini and this package should be renamed to @unocss/extractor-arbitrary like #2368.

variantVariables has preset-mini specific code and has nothing to do with extracting. Otherwise we have to move cssProperty rule here too and having preset-mini dependency in an extractor package doesn't make sense:

unocss/packages/preset-mini/src/_rules/variables.ts

Lines 28 to 33 in 8554bf4

export const cssProperty: Rule[] = [

[/^\[(--(\w|\\\W)+|[\w-]+):([^\s:]*?("\S+?"|'\S+?'|`\S+?`|[^\s:]+?)[^\s:]*?\)?)\]$/, ([match, prop,, value]) => {

if (!isURI(match.slice(1, -1)))

return { [prop]: h.bracket(`[${value}]`) }

}],

]

And by doing that we can include arbitrary extractor in preset-mini exctractors and avoid breaking change.

antfu · 2023-03-18T20:01:41Z

So, instead of moving arbitrary extractor to a separate package, we should move it to preset-mini

That sounds good to me. Initially, I was thinking of enabling arbitrary variants also for other presets. Directly embedding could also do.

hooks

That's probably not a bad idea. But I think we won't implement all hooks you mentioned at once. Instead, we could implement them when we see the needs, so we don't over-engineer.

chu121su12 · 2023-03-20T01:17:06Z

So, instead of moving arbitrary extractor to a separate package, we should move it to preset-mini

While at it, I think the variants on preset-mini may benefit when split on separate package

feat(preset-arbitrary): make arbitrary variant standalone

1d6b78a

antfu changed the title ~~feat(preset-arbitrary): make arbitrary variant standalone~~ feat(preset-arbitrary)!: make arbitrary variant standalone Mar 17, 2023

antfu added the breaking change label Mar 17, 2023

chore: update

72b40d6

sibbng requested changes Mar 17, 2023

View reviewed changes

sibbng reviewed Mar 17, 2023

View reviewed changes

antfu added 2 commits March 18, 2023 12:03

chore: update

64ed892

feat: add configResolved

350fd1a

chore: update bench fixture

aa5e7d4

sibbng requested changes Mar 18, 2023

View reviewed changes

antfu added 2 commits April 5, 2023 09:11

Merge branch 'main' into feat/preset-arbitrary

4052064

fix: provide as extractor

e30a13c

antfu changed the title ~~feat(preset-arbitrary)!: make arbitrary variant standalone~~ feat(extractor-arbitrary-variants)!: make arbitrary variant standalone Apr 5, 2023

antfu changed the title ~~feat(extractor-arbitrary-variants)!: make arbitrary variant standalone~~ feat(extractor-arbitrary-variants)!: make arbitrary extractor standalone Apr 5, 2023

antfu added 2 commits April 5, 2023 09:41

refactor: revert test changes

326591e

chore: update

c97de81

antfu marked this pull request as draft April 5, 2023 07:56

antfu added 2 commits April 8, 2023 11:40

Merge branch 'main' into feat/preset-arbitrary

a433719

Merge branch 'main' into feat/preset-arbitrary

ebd5b55

antfu changed the title ~~feat(extractor-arbitrary-variants)!: make arbitrary extractor standalone~~ feat(preset-mini)!: move arbitrary extractor to preset-mini Apr 9, 2023

antfu changed the title ~~feat(preset-mini)!: move arbitrary extractor to preset-mini~~ feat(extractor-arbitrary-variants)!: make arbitrary extractor standalone Apr 9, 2023

antfu added 2 commits April 9, 2023 08:54

Merge branch 'main' into feat/preset-arbitrary

2313a88

chore: refactor

4c24b3a

antfu added 2 commits April 9, 2023 10:15

Merge branch 'main' into feat/preset-arbitrary

6360571

chore: update

6ff5bac

antfu marked this pull request as ready for review April 9, 2023 08:32

antfu merged commit 8baeaf8 into main Apr 9, 2023
9 checks passed

antfu deleted the feat/preset-arbitrary branch April 9, 2023 08:32

renovate bot mentioned this pull request Oct 21, 2023

chore(deps): update unocss monorepo to v0.57.1 upjs/facile-validator#346

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(extractor-arbitrary-variants)!: make arbitrary extractor standalone #2364

feat(extractor-arbitrary-variants)!: make arbitrary extractor standalone #2364

antfu commented Mar 17, 2023 •

edited

netlify bot commented Mar 17, 2023 •

edited

sibbng Mar 17, 2023

sibbng Mar 17, 2023

antfu Mar 18, 2023

sibbng Mar 17, 2023 •

edited

sibbng Mar 17, 2023

sibbng Mar 17, 2023

antfu Mar 18, 2023

antfu Mar 18, 2023

sibbng left a comment

antfu commented Mar 18, 2023

antfu commented Mar 18, 2023 •

edited

sibbng commented Mar 18, 2023 •

edited

sibbng Mar 18, 2023 •

edited

sibbng Mar 18, 2023 •

edited

sibbng Mar 18, 2023

sibbng Mar 18, 2023 •

edited

antfu commented Mar 18, 2023

chu121su12 commented Mar 20, 2023

+                        [
+,
+,
+                          "b",
+                        ],

	export const cssProperty: Rule[] = [
	[/^\[(--(\w\|\\\W)+\|[\w-]+):([^\s:]?("\S+?"\|'\S+?'\|`\S+?`\|[^\s:]+?)[^\s:]?\)?)\]$/, ([match, prop,, value]) => {
	if (!isURI(match.slice(1, -1)))
	return { [prop]: h.bracket(`[${value}]`) }
	}],
	]

feat(extractor-arbitrary-variants)!: make arbitrary extractor standalone #2364

feat(extractor-arbitrary-variants)!: make arbitrary extractor standalone #2364

Conversation

antfu commented Mar 17, 2023 • edited

netlify bot commented Mar 17, 2023 • edited

✅ Deploy Preview for unocss ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sibbng Mar 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sibbng left a comment

Choose a reason for hiding this comment

antfu commented Mar 18, 2023

antfu commented Mar 18, 2023 • edited

sibbng commented Mar 18, 2023 • edited

sibbng Mar 18, 2023 • edited

Choose a reason for hiding this comment

sibbng Mar 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sibbng Mar 18, 2023 • edited

Choose a reason for hiding this comment

antfu commented Mar 18, 2023

chu121su12 commented Mar 20, 2023

antfu commented Mar 17, 2023 •

edited

netlify bot commented Mar 17, 2023 •

edited

sibbng Mar 17, 2023 •

edited

antfu commented Mar 18, 2023 •

edited

sibbng commented Mar 18, 2023 •

edited

sibbng Mar 18, 2023 •

edited

sibbng Mar 18, 2023 •

edited

sibbng Mar 18, 2023 •

edited