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

feat(core)!: add content option #2503

Closed
wants to merge 4 commits into from
Closed

feat(core)!: add content option #2503

wants to merge 4 commits into from

Conversation

sibbng
Copy link
Member

@sibbng sibbng commented Apr 14, 2023

TODO:

  • explain the motivation
  • add tests

Breaking changes:

  • postcss!: content option removed
  • core: content option added (similar to https://tailwindcss.com/docs/content-configuration)
  • core!: include and exclude options moved to content option
  • core!: extraContent removed, functionality moved to content option

@netlify
Copy link

netlify bot commented Apr 14, 2023

Deploy Preview for unocss ready!

Name Link
🔨 Latest commit c4cc331
🔍 Latest deploy log https://app.netlify.com/sites/unocss/deploys/643b530bf5754d0008cb7390
😎 Deploy Preview https://deploy-preview-2503--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 15, 2023

I am ok with this refactoring to make options more scoped. But I think we need to make it clear that UnoCSS works differently from Tailwind, that we read "content" from the build pipeline, not from the filesystem manually. So I'd suggest we drop the shorthand of providing an array directly, and name files as filesystem or so.

{
  content: {
    filesystem: ['**/foo/**.vue'],
    pipeline: {
       include: [],
       exclude: []
    }
  }
}
{
  content: {
    filesystem: {
      glob: ['**/foo/**.vue'],
      exclude: [] // tho I doubt if this is necessary
    },
    pipeline: false // disable reading from pipeline, relying on fs solely,
    text: [`some content to extra`] 
  }
}

@sibbng
Copy link
Member Author

sibbng commented Apr 15, 2023

UnoCSS works differently from Tailwind, that we read "content" from the build pipeline, not from the filesystem manually

This is only true for Vite plugin, extraContent option and PostCSS plugin read files from the filesystem. And I don't think end users have to know that until they really need to.

But I think we need to make it clear...

We should make it clear only on documentation. I don't think end users will ever need to know that until they have to deal with query parameters that passed along paths. For example:

  • example.vue?macros=true
  • example.svelte?svelte&type=style&lang.css

These patterns will not be matched by any glob patterns. Because glob patterns compiled to something like that: /^.+\.vue$/. This is the only scenario I can think of when people will ever need to use include and exclude options.

Also, I would like the mention one thing I did that may not be obvious from the changes. Glob patterns also passed to include filter and I removed string type from type FilterPattern to encourage users to use files option instead. This makes clear include & exclude options are for advanced use cases, and most of the time used by us rather than users.

So I'd suggest we drop the shorthand of providing an array directly

I'm strongly against that suggestion. Most of the time people will not ever need to configure this option. When they have to, simply providing an array of glob patterns seems pleasing to me as a user. And Tailwind team thinking of making content optional on v4. We shouldn't worry too much about that.

My goals with these changes were:

  • Unify content options, so Vite plugin and PostCSS can rely on the same options
  • Make migration from/to Tailwind easier
  • Cleaner content options: Tailwind team nailed that. It is super clear from their documentation how content extraction work. In UnoCSS project I have seen a lot of issues/discussions from people that confused about how extraction work.

@antfu
Copy link
Member

antfu commented Apr 15, 2023

This is only true for Vite plugin

Actually, this is part of the design phoilshpy of UnoCSS, is that we could do that whenever possible. I understand it's not possible for PostCSS plugin, and I think that makes sense. But in that sense, it's actually "This is only false for PostCSS plugin".

Unify content options, so Vite plugin and PostCSS can rely on the same options

So you see I am trying to make the interface generic for both cases. You could say content.pipeline is not available in PostCSS, where content.filesystem works for both.

drop the shorthand of providing an array directly > Most of the time people will not ever need to configure this option

If we say most of ppl don't need it, then it actually makes less sense to me to make it "easier". My question is that if we put content: ['somefile/*.ts'], what does that mean to a Vite plugin? Is that saying we:

  • only extract from that file
  • apply it as a include filter to the pipeline
  • or adding the file on top of the pipeline extracting

Either way, I think they will have different semantic meanings across Vite and PostCSS.

I admit that "extraContent" can be a bit not fair to PostCSS, so I agree with this renaming to make it general.

In UnoCSS project I have seen a lot of issues/discussions from people that confused about how extraction work.

Maybe we are biased? People would ask simple questions without looking into the docs anyway, they would keep asking why bg-${color} does work, even if it's the same limitation of Tailwind or Windi.

@sibbng
Copy link
Member Author

sibbng commented Apr 15, 2023

@antfu I updated PR with your suggestions. There is still some work to do. I just want to finalize the API.

This is the current type of content option:

export type FilterPattern = ReadonlyArray<string | RegExp> | string | RegExp | null

export interface ConfigBase<Theme extends {} = {}> {
  content?: {
    filesystem?: string[]
    plain?: (string | { content: string; extension: string })[]
    pipeline?: {
      include?: FilterPattern
      exclude?: FilterPattern
    }
  }
}

Comment on lines +145 to +147
// @ts-expect-error private
if (config.__postcss)
content.filesystem ||= defaultIncludeGlobs
Copy link
Member

Choose a reason for hiding this comment

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

Could this be outside of the core? Or maybe the entire content config could be outside and up to the integrations to resolve

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I will move it to the PostCSS plugin.

/**
* Patterns that filter the files being extracted.
*/
include?: FilterPattern
Copy link
Member

Choose a reason for hiding this comment

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

We better keep those types, but make as deprecated to say "use xxx instead"

export const defaultInclude: Exclude<FilterPattern, null> = [/\.(vue|svelte|[jt]sx|mdx?|astro|elm|php|phtml|html)($|\?)/]

// micromatch patterns, used in postcss plugin and createFilter
export const defaultIncludeGlobs = ['./**/*.{html,js,ts,jsx,tsx,vue,svelte,astro,elm,php,phtml,mdx,md}']
Copy link
Member

Choose a reason for hiding this comment

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

I feel they are better to live in shared-integrations

Copy link
Member Author

Choose a reason for hiding this comment

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

I was getting an error during the production build. This is why I moved them there. I will give it a look again.

/**
* Plain text to be extracted
*/
plain?: (string | { content: string; extension: string })[]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
plain?: (string | { content: string; extension: string })[]
plain?: (string | { content: string; id: string })[]

Maybe we could name it id to align with the pipeline and allow to pass a path

Copy link
Member Author

Choose a reason for hiding this comment

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

I wouldn't suggest doing that. It would be confusing in many ways. Let's say we have 2 plain content objects with id property. What id should be? A path, relative or absolute, should it exist in fs or just an extension? And most importantly ids should be unique, This will force people to think of a unique name for their ids. We somewhat forcing them to overthink. extension name clearly shows all we need is an extension.

Copy link
Member

Choose a reason for hiding this comment

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

It's sometimes about extensions. Extractors can check the module id and apply conditionally based on it (under certain folders, or conventions). For example, in Vue we will have

  • foo.vue
  • foo.vue?vue&type=template&lang=pug
  • foo.vue?vue&type=style&lang=css

If I want to apply transformation before passing the extractor, I will need to compile the pug template to HTML, in this case, extension can be limited.

Maybe this would not be used often, but being able to pass the full path (it's always named as id in other places) would make it extendable in the future and more consistent across the pipeline.

If you are concerned about the understanding, I think we could document to explain it in jsdocs or the docs site.

plain?: (string | { content: string; extension: string })[]

/**
* Options used by Vite to filter processing files.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Options used by Vite to filter processing files.
* Filter to determine whether to extract certain modules from the build tools' transformation pipeline

Comment on lines +317 to +325
/**
* Glob patterns to extract th classes from.
*
* In Vite plugin, this patterns only used to extract classes
* from the files that is not part of the build pipeline.
*
* In PostCSS plugin, files to be scanned are determined by
* the glob patterns defined in this option.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* Glob patterns to extract th classes from.
*
* In Vite plugin, this patterns only used to extract classes
* from the files that is not part of the build pipeline.
*
* In PostCSS plugin, files to be scanned are determined by
* the glob patterns defined in this option.
*/
/**
* Glob patterns to extract from the file system.
*/

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO we should clarify its behavior in Vite. Otherwise, people will be confused about why transform plugins are not working on those files.

Copy link
Member

Choose a reason for hiding this comment

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

What I am thinking it that the content itself indicates multiple sources to read from. We could say pipeline source is only supported in Vite and Webpack but not in PostCSS, etc.

@@ -313,6 +313,39 @@ export type Postprocessor = (util: UtilObject) => void
export type ThemeExtender<T> = (theme: T) => T | void

export interface ConfigBase<Theme extends {} = {}> {
content?: {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
content?: {
/**
* Sources of the content of UnoCSS to extract from.
* The support of the sources depends on the integration you are using.
* Please refer to their documentation to learn the usage.
*/
content?: {

@antfu
Copy link
Member

antfu commented May 21, 2023

I would love to help moving this forward, let me how I can help on it.

@sibbng
Copy link
Member Author

sibbng commented May 23, 2023

I would love to help moving this forward, let me how I can help on it.

Todos:

  • Resolve content options in the Nuxt package
  • Remove config.__postcss
  • Add runtime warnings for deprecated APIs

By extracting the changes in the packages/core/src/config.ts to shared-integration package we can resolve default values for nuxt and postcss packages from there. This may also eliminate the required changes related to ResolvedConfig type (checkpackages/shared-integration/src/context.ts).

@antfu antfu changed the title refactor(core)!: add content option feat(core)!: add content option May 23, 2023
@antfu antfu closed this in #2719 Jun 4, 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

2 participants