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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(svelte-scoped)!: rewrite svelte-scoped mode #2530

Closed
wants to merge 1 commit into from

Conversation

devunt
Copy link
Contributor

@devunt devunt commented Apr 20, 2023

Existing implementation of svelte-scoped plugin has some flaws such as incapability of parsing complex svelte expression strings. This PR thoroughly rewrites svelte-scoped plugin transformation code and make some optimization.

Notable, and possible breaking changes

  • Overall transformation pipeline is greatly simplified for code readability and performance improvement.
  • Drop attributify mode, which was buggy and had negative effect on performance and should be handled in the separated plugin.
  • Shortcuts will be hashed as the same as rest of other utilities. It makes generated code more consistent and works in an expected manner. It's new behavior is also aligned with @unocss/transformer-compile-class, which compiles everything including user-defined shortcuts to a single generated class.
  • Class hashes will be consistent between production and development mode. In before, the same px-2 py-1 class will be hashed into uno-nv0d81 in production and _px-2_8thd01 _py-1_8thd01 in development because different hash algorithm was used. It will now hashed into _uno-nv0d81__px-2 _uno-nv0d81__py-1 in development, make it deterministic between production and development environment. Developers will be able to know what will be the actual compiled class name in the production while developing, and will be able to track down production code to development code by comparing the hashed class names.

Bug fixes

  • Some utilities was not generated when handling complex expressions in class= attribute.
  • Class directives wrapped in quotes such as class:flex="{flex}" will be correctly handled.
  • Existing style tag attributes will be preserved such as lang="scss".

Questions

  • Still not sure it is a good decision to drop attributify support. Although it was somewhat buggy, degrading performance (previous implementation was really more like workaround), and non-standard in svelte (typescript and svelte-check will emit error if non-standard html attributes are used in the code), there is a some demand for it and I'm not a great fan of making user-facing breaking change.
  • Since this PR has some opinionated changes, I'm open for any discussions.

@devunt devunt requested a review from antfu as a code owner April 20, 2023 20:22
@netlify
Copy link

netlify bot commented Apr 20, 2023

Deploy Preview for unocss ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit e49f0dd
🔍 Latest deploy log https://app.netlify.com/sites/unocss/deploys/6441a078e294f500083daf4f
😎 Deploy Preview https://deploy-preview-2530--unocss.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@antfu
Copy link
Member

antfu commented Apr 22, 2023

I am not very familiar with Svelte. @jacob-8 can you help review it? Thanks

@jacob-8
Copy link
Contributor

jacob-8 commented Apr 23, 2023

I can review the code in two days when I'm back at my desk, but first I want to ask, @antfu, what level of support do you hope to give to svelte-scoped mode in this repo? For both technical reasons and the need for ability to make updates as needed (explained in #2098 (comment)) I turned svelte-scoped into a separate project that depends on UnoCSS.

That being said, the svelte-scoped mode in this UnoCSS as it currently stands is half baked. I mentioned in #2098 that I would like to create a PR that helps clarify the Uno docs as to the current state of the svelte-scoped. I'm still waiting to know your opinion on what that PR should do.

I think it would be simplest for users for me to remove svelte-scoped from this repo and add instructions to use the svelte-scoped-uno package. I also don't want to maintain svelte-scoped in two locations as the feature set in svelte-scoped-uno has already improved and this PR here in based on an older version. That's my preference but if you have another preference please let me know.

@sibbng
Copy link
Member

sibbng commented Apr 23, 2023

Drop attributify mode, which was buggy and had negative effect on performance and should be handled in the separated plugin.

Hi, I'm the guy who added attributify support. While working on that feature I'm a bit disappointed to see the way svelte-scoped mode is implemented. I wasn't proud of the code I wrote. I followed the pattern that was already there.

I'm totally fine with the removal of attributify support. I'm not a fan of it, I don't use it. I just wanted to close the gap between modes.

IMO svelte-scoped mode could have been 100x times more stable and readable if it was using svelte/compiler and AST walk. I wanted to explore this approach but unfortunately, I haven't had time.

I'm working on some sort of scoped feature for our PostCSS plugin that can also be supported by the Vite plugin in the future. I have already created a POC locally. No transformation, no Vite plugin, vite-plugin-svelte does the scoping and hashing for us. It works great. I will make a proposal about it ASAP.

@devunt
Copy link
Contributor Author

devunt commented Apr 23, 2023

First of all, I want to say that absolutely no offense was intended to prior codes and its contributors. I apologize for any impoliteness that may have been introduced by my bad English skill. I have no doubt that the previous approach was the best at the moment it was written, but simply thought there was a space for improvement.

IMO svelte-scoped mode could have been 100x times more stable and readable if it was using svelte/compiler and AST walk. I wanted to explore this approach but unfortunately, I haven't had time.

I totally agree with this. As I have some experience with Svelte AST (I originally wrote the eslint-plugin-unocss and also had added Svelte support for it in #2417), this approach is more comfortable for me, too.

@jacob-8
Copy link
Contributor

jacob-8 commented Apr 23, 2023

svelte-scoped mode could have been 100x times more stable and readable if it was using svelte/compiler and AST walk

If the community wants to use the Svelte AST and develop more powerful features from that, I understand if a case can be made for them. However, I'm under the impression from Uno's docs that avoiding using ASTs leads to better performance: "Instant: No parsing, no AST, no scanning. It’s 5x faster than Windi CSS or Tailwind JIT." I think that refactoring and cleaning up the code, and not using ASTs are the solution for improvement here.

@devunt no offense taken, most of the code messiness is my fault, the improving of which is why I moved to a separate repo to allow for faster iteration and more tests. Unit tests allow for easier refactoring and improved readability and stability.

As to attributify mode, I never intended to support that. If I may venture a guess I feel like Vue devs like to do a lot of cool things like auto imports and attributify whereas Svelte leans more towards making things more explicit. Keep the imports clear, keep the class names inside of class attributes and such. You can certainly try attributify if you want but it feels off to me. Just my opinion, certainly not a rule.

@jacob-8
Copy link
Contributor

jacob-8 commented Apr 23, 2023

Some utilities was not generated when handling complex expressions in class= attribute.

Can you please add some examples here as a basis for future tests? By the way, it's a best practice to not use conditional logic inside the traditional class= attribute. Svelte encourages the use of class:mb-1={variable} for that.

Class directives wrapped in quotes such as class:flex="{flex}" will be correctly handled.

This is incorrect syntax and shouldn't be supported.

@antfu
Copy link
Member

antfu commented Apr 23, 2023

Hi @jacob-8 thanks for the work and I am sorry that I missed your comments in the previous PRs. I think the blocker for me is clear - I don't use Svelte and I don't have enough knowledge to review them. That said I do agree it would be better if we drop it in the core and serve it as a separate package.

If there have enough interest in this and you do think it could be better to have it close to the core, I'd be happy to have @jacob-8 on the team and move the Svelte package as an official. As long as we have at least two members (maybe @jacob-8 + @sibbng?) working on Svelte that can peer-review each other's code.

Otherwise, we could remove the code and update the readme to suggest svelte-scoped-uno.

@jacob-8
Copy link
Contributor

jacob-8 commented Apr 23, 2023

I am sorry that I missed your comments in the previous PRs

No need to apologize, @antfu, I'm busy using all your tools that I know you are busy building. 😆 It's not fair to expect you to whip through everything so I appreciate and accept your offer to join if each of the following is suitable with your plans for UnoCSS:

  • svelte-scoped-uno is brought back in as its own package at packages/svelte-scoped
  • I can co-locate tests with functions within the package (ie smallFunction.ts and smallFunction.test.ts). I don't see you doing this elsewhere in UnoCSS but I feel it's needed to help me make some necessary refactoring, and to encourage more thorough testing.

@fehnomenal may also be interested in being involved as he understands all the code so far.

As to the svelte-preprocess-unocss needed only for library authors because Vite is not part of the svelte-package pipeline, I can keep that external as per the wishes of other maintainers not wanting to bloat the UnoCSS repo.

@antfu
Copy link
Member

antfu commented Apr 23, 2023

Can you share a bit more about how do you compare with the vite plugin approach and svelte preprocessor?

If the Svelte preprocessor exposes AST and other information, I feel it's better to reuse them to provide more accurate transformations. Also eliminates the confusion of two Vite plugins if it's able to provide feature parity of the Vite plugin (also decoupling from Vite would be a nice addition). Is there any limitation of the preprocessor?

@jacob-8
Copy link
Contributor

jacob-8 commented Apr 23, 2023

We still have some global styles that are placed into the head tag before Svelte component styles are added. At the moment this includes resets, preflights, safelist, certain plugins like prose. Currently these are injected using Vite's transformIndexHtml hook in vanilla Svelte and the Vite plugin facilitates using the transformPageChunk hook in hooks.server.ts in SvelteKit. So in SvelteKit at least I guess we don't need Vite for what I've mentioned so far. Perhaps we just need to expose a function to be used in hooks.server.ts.

So I guess what remains of a Vite plugin being useful then is any sort of server interaction that could be built out in the future (ie inspector). But yes, the actual component transformation just be done from a svelte preprocessor, that's a great idea to just use the already created Svelte AST.

Let me think on this... so it sounds like we don't need a Vite plugin right now but may later for dev server interaction benefits. In that case Vite plugins do have a field to accept a Svelte preprocessor and just pass it along to the regular preprocess pipeline.

@jacob-8
Copy link
Contributor

jacob-8 commented Apr 23, 2023

Perhaps we just need to expose a function to be used in hooks.server.ts.

Just remembered, as can be seen in
https://github.com/jacob-8/svelte-scoped-uno/blob/main/packages/svelte-scoped-uno/src/globalStylesPlugin.ts we are using Vite to create a hashed cacheable css file on build for those aforementioned global items. That would be lost and we would have to settle for always inlining.

@antfu
Copy link
Member

antfu commented Apr 23, 2023

@jacob-8 Invited you to the team, and feel free to start working on the refactoring. I am ok with co-located test files. About the package name, I am not very sure about @unocss/svelte-scoped as it feels a bit leak of context - or maybe we could have sub-entries like @unocss/svelte-scoped/vite and @unocss/svelte-scoped/preprocessor?

@devunt @sibbng Feel free to raise any discussions together with @jacob-8, and let's see how it could go.

Thanks to everyone involved!

@jacob-8
Copy link
Contributor

jacob-8 commented Apr 24, 2023

Thank you! Give me a couple days and I'll be back home and implementing things.

@henrikvilhelmberglund
Copy link
Contributor

By the way, it's a best practice to not use conditional logic inside the traditional class= attribute. Svelte encourages the use of class:mb-1={variable} for that.

Right now you can only have one class per directive which makes it not ideal for things like UnoCSS, so if you don't want to use shortcuts you're forced to use conditionals inside class=.

There is https://github.com/fernandolguevara/svelte-multicssclass but it's not compatible with UnoCSS right now.

@jacob-8
Copy link
Contributor

jacob-8 commented Apr 28, 2023

if you don't want to use shortcuts you're forced to use conditionals inside class=

I used to use conditionals inside class= quite a bit and stopped for two reasons.

  1. Tooling doesn't always handle it gracefully. I've had experience where the Svelte Language Server chokes on it, or where Prettier mangles my code beyond readability. I think many of these problems get ironed out, but this seem to be an ongoing source of potential future problems.

  2. They are hard to read when you come back later and try to figure out which classes show when. If you only have two or three classes being toggled, it's much more clear if you have class:mb-1={foo} for each. If you have many then using --at-apply (via transformerDirectives) is a good option:

<div class:my-custom-class={foo} />

<style>
  .my-custom-class {
    --at-apply: text-red font-semibold mx-2 my-1 p-2;
  }
</style>

https://github.com/fernandolguevara/svelte-multicssclass

Thanks for linking, @henrikvilhelmberglund - this is an interesting idea. Not sure what I think of it at the moment but certainly a creative approach. Edit: svelte-multicssclass conversation continued in #2322

@antfu antfu marked this pull request as draft May 5, 2023 16:11
@jacob-8
Copy link
Contributor

jacob-8 commented May 8, 2023

I'm making good progress on #2552 and wanted to leave some notes regarding the code changes made in this pr as well as the conversation thread.

Using AST for parsing?

If the Svelte preprocessor exposes AST and other information, I feel it's better to reuse them to provide more accurate transformations.

The Svelte preprocessor does not expose an AST, it just gives three different editing pipelines where you get either 1) entire file, 2) script block contents, or 3) style block contents, which you can transform and hand back . It makes sense that they do not give an AST as you can have an unlimited number of preprocessors, for which you'd need to create a new AST between each step. That's too expensive.

svelte-scoped mode could have been 100x times more stable and readable if it was using svelte/compiler and AST walk

Most of the complexity comes from the nature of the task we are tackling (distinguishing between supported utility rules, random unrelated classes the user writes in themselves, shortcuts, generating hashed names, etc...) than from using AST vs regex to find out which classes exist in the component. As you can see from #2552, things are much more readable, testable, and updateable than before.

Note that the returned AST is not considered public API, so breaking changes could occur at any point in time. (from https://svelte.dev/docs#compile-time-svelte-parse)

The first reason I would rather not use the AST is that the Svelte maintainers have not committed to a stable API. To me the complexity of breaking changes occuring at any moment and then having to maintain multiple versions of our code does not sound fun. The likelihood of this actually happening is small, but still not something I want to lean into if they are discouraging it.

The second and main reason is that AST parsing is slow. Now that my code updates are finished except for some minor variable renaming for clarity, I created 3 sample Svelte components and ran some benchmarks:

image

In each we are comparing the speed of:
A) just creating an AST, nothing else
B) using regex to find all the classes (locations and content of all regular class="..." (including details for inline expressions), class:mb1-={foo}, and class:logo) and then processing those classes (calculating which classes are Uno related, generating hashed names for those and preparing to hand off to the Uno generator while leaving alone classes not related to Uno)

So it's not really an equal comparison because no one has written the class processing using the AST method, but the results are still insightful considering the tests are skewed in favor of the AST.

In the first ridiculously simple component of 8 lines, the regex route is 18x faster. We probably already knew this.

In the second component I created an artificially large number of classes vs lines than would be normal (about 55 classes of each type--uno, shortcut, and unknown--in only 29 lines). This was an attempt to equal them out but the regex route is still 2x faster than just creating an AST.

The last component is one I actually use, a slideover with 80 lines including a lot of logic as well as utility classes. This is a more typical balance of classes encountered to lines of code. The regex is 8x faster.

Mostly I did this research for myself, so I could understand AST speeds a little better (and give Vitest's bench feature a try 😁) but I think it's helpful for our decisions here. Of course, after #2552 is merged, anyone with the desire is welcome to write the transformation using an AST instead of regex and create a PR with benchmarks showing how it is a viable option. I don't think it can compare but then again, my AST usage experience is not that much and I wouldn't mind having my suspicions proven wrong.

hashing shortcuts

I wanted to keep these simple and not hash, but yes they need to be and are now. Thanks for bringing this up!

Attributify mode

The svelte-scoped docs will include a note to use the unplugin-attributify-to-class Vite plugin and just update the include option: attributifyToClass({ include: [/\.svelte$/]}). Since a Vite plugin already does the needed work I think we should just point users to that if they want attributify mode. If someone wants it as a svelte preprocessor instead of a Vite plugin, I think it would be trivial for them to create their own modeled on that plugin.

class hashes

Class hashes will be consistent between production and development mode

Having a distinction is an intentional feature that's very useful. It's helpful in dev tools while developing to have class names separated out and at the beginning of a class name (mb-1_eidjei, not eidjei_mb-1) for easy reading and toggling on and off. It's also nice in build to have things a bit more compact. Because the Vite plugin can detect dev/build it will automatically make the distinction, however it will be trivial for users who don't like the default behavior to always choose one or the other. When using the Svelte preprocessor by itself, it defaults to combining classes as it has no way to detect dev/build context. The docs will include a small snippet that can be added to the svelte.config.js file if user's want the distinction between dev/build.

Supported ways of writing classes

I already replied previously a bit about different ways to write classes, but here's a summary of supported methods:

  • regular: class="mb-1 mr-1"
  • class directives: class:mb-1={variable} and the shorthand: class:logo
  • expressions: class="mb-1 { variable ? 'mr-1' : 'ml-1' }" - I used to use these and now strongly discourage the use of them for a number of reasons related to readability and search among other things. I think it's much better to use class: but because people commonly use expressions and to make it easier to migrate from Tailwind or regular global Uno usage, we support them.

For those wanting to lean into using class directives for conditionally applying a string of classes, using --at-apply/@apply or the above mentioned https://github.com/fernandolguevara/svelte-multicssclass can help.

style attributes bug

Existing style tag attributes will be preserved such as lang="scss"

Don't know when this was an issue but it is not anymore.


With the above discussed suggestions and improvements being implemented as approved in #2552, I'll close this pr. Please continue discussion here or on that PR for items that people want to discuss further.

@jacob-8 jacob-8 closed this May 8, 2023
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

5 participants