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

Resolve @import in core #14446

Merged
merged 51 commits into from
Sep 23, 2024
Merged

Resolve @import in core #14446

merged 51 commits into from
Sep 23, 2024

Conversation

philipp-spiess
Copy link
Member

@philipp-spiess philipp-spiess commented Sep 17, 2024

This PR brings @import resolution into Tailwind CSS core. This means that our clients (PostCSS, Vite, and CLI) no longer need to depend on postcss and postcss-import to resolve @import. Furthermore this simplifies the handling of relative paths for @source, @plugin, or @config in transitive CSS files (where the relative root should always be relative to the CSS file that contains the directive). This PR also fixes a plugin resolution bug where non-relative imports (e.g. directly importing node modules like @plugin '@tailwindcss/typography';) would not work in CSS files that are based in a different npm package.

Resolving @import

The core of the @import resolution is inside packages/tailwindcss/src/at-import.ts. There, to keep things performant, we do a two-step process to resolve imports. Imagine the following input CSS file:

@import "tailwindcss/theme.css";
@import "tailwindcss/utilities.css";

Since our AST walks are synchronous, we will do a first traversal where we start a loading request for each @import directive. Once all loads are started, we will await the promise and do a second walk where we actually replace the AST nodes with their resolved stylesheets. All of this is recursive, so that @import-ed files can again @import other files.

The core @import resolver also includes extensive test cases for various combinations of media query and supports conditionals as well als layered imports.

When the same file is imported multiple times, the AST nodes are duplicated but duplicate I/O is avoided on a per-file basis, so this will only load one file, but include the @theme rules twice:

@import "tailwindcss/theme.css";
@import "tailwindcss/theme.css";

Adding a new context node to the AST

One limitation we had when working with the postcss-import plugin was the need to do an additional traversal to rewrite relative @source, @plugin, and @config directives. This was needed because we want these paths to be relative to the CSS file that defines the directive but when flattening a CSS file, this information is no longer part of the stringifed CSS representation. We worked around this by rewriting the content of these directives to be relative to the input CSS file, which resulted in added complexity and caused a lot of issues with Windows paths in the beginning.

Now that we are doing the @import resolution in core, we can use a different data structure to persist this information. This PR adds a new context node so that we can store arbitrary context like this inside the Ast directly. This allows us to share information with the sub tree while doing the Ast walk.

Here's an example of how the new context node can be used to share information with subtrees:

const ast = [
  rule('.foo', [decl('color', 'red')]),
  context({ value: 'a' }, [
    rule('.bar', [
      decl('color', 'blue'),
      context({ value: 'b' }, [
        rule('.baz', [decl('color', 'green')]),
      ]),
    ]),
  ]),
]

walk(ast, (node, { context }) => {
  if (node.kind !== 'declaration') return
  switch (node.value) {
    case 'red':   assert(context.value === undefined)
    case 'blue':  assert(context.value === 'a')
    case 'green': assert(context.value === 'b')
  }
})

In core, we use this new Ast node specifically to persist the base path of the current CSS file. We put the input CSS file base at the root of the Ast and then overwrite the base on every @import substitution.

Removing the dependency on postcss-import

Now that we support @import resolution in core, our clients no longer need a dependency on postcss-import. Furthermore, most dependencies also don't need to know about postcss at all anymore (except the PostCSS client, of course!).

This also means that our workaround for rewriting @source, the postcss-fix-relative-paths plugin, can now go away as a shared dependency between all of our clients. Note that we still have it for the PostCSS plugin only, where it's possible that users already have postcss-import running before the @tailwindcss/postcss plugin.

Here's an example of the changes to the dependencies for our Vite client ✨ :

Screenshot 2024-09-19 at 16 59 45

Performance

Since our Vite and CLI clients now no longer need to use postcss at all, we have also measured a significant improvement to the initial build times. For a small test setup that contains only a hand full of files (nothing super-complex), we measured an improvement in the 3.5x range:

Screenshot 2024-09-19 at 14 52 49

The code for this is in the commit history if you want to reproduce the results. The test was based on the Vite client.

Caveats

One thing to note is that we previously relied on finding specific symbols in the input CSS to bail out of Tailwind processing completely. E.g. if a file does not contain a @tailwind or @apply directive, it can never be a Tailwind file.

Since we no longer have a string representation of the flattened CSS file, we can no longer do this check. However, the current implementation was already inconsistent with differences on the allowed symbol list between our clients. Ideally, Tailwind CSS should figure out wether a CSS file is a Tailwind CSS file. This, however, is left as an improvement for a future API since it goes hand-in-hand with our planned API changes for the core tailwindcss package.

@philipp-spiess philipp-spiess force-pushed the feat/at-import-resolver branch 5 times, most recently from 95d9682 to c3f7963 Compare September 19, 2024 13:25
@philipp-spiess philipp-spiess marked this pull request as ready for review September 19, 2024 15:10
Copy link
Contributor

@thecrypticace thecrypticace left a comment

Choose a reason for hiding this comment

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

Dude this looks 💯 — a handful of questions and comments about stuff but dang this is some epic work.

packages/@tailwindcss-vite/src/normalize-path.ts Outdated Show resolved Hide resolved
packages/@tailwindcss-vite/src/index.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/compat/plugin-api.test.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/index.ts Outdated Show resolved Hide resolved

// Find all `@theme` declarations
let theme = new Theme()
let customVariants: ((designSystem: DesignSystem) => void)[] = []
let customUtilities: ((designSystem: DesignSystem) => void)[] = []
let firstThemeRule: Rule | null = null
let keyframesRules: Rule[] = []
let globs: { origin?: string; pattern: string }[] = []
let globs: { base: string; pattern: string }[] = []
Copy link
Contributor

Choose a reason for hiding this comment

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

this makes me happy 🥳

packages/tailwindcss/src/index.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/index.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/index.ts Outdated Show resolved Hide resolved
@thecrypticace
Copy link
Contributor

thecrypticace commented Sep 19, 2024

Also was thinking but you can eliminate the 2nd walk by making use of object identity. Basically you'll eagerly create the context node, store functions that load the stylesheet, parse it, etc… and then replace the nodes / metadata of the context node when it's done loading.

Then you call all the promises after you've done the walk and once they've resolved the AST has been updated.

    let ctx = context({ base: '.' }, [])
    replaceWith(ctx)

    if (promises.has(uri)) return

    promises.set(uri, async () => {
      const imported = await loadStylesheet(uri, base)
      let root = CSS.parse(imported.content)
      await substituteAtImports(root, imported.base, loadStylesheet)

      if (layer !== null) {
        root = [rule('@layer ' + layer, root)]
      }

      if (media !== null) {
        root = [rule('@media ' + media, root)]
      }

      if (supports !== null) {
        root = [rule(`@supports ${supports[0] === '(' ? supports : `(${supports})`}`, root)]
      }

      ctx.context = { base: imported.base }
      ctx.nodes = root
    })

@thecrypticace
Copy link
Contributor

A realization I just had while prepping Prettier for this change — the current definition of loadModule is insufficient. For the purposes of Intellisense (and probably Prettier) I need to to know what kind of thing is being loaded so I can stub in a value that doesn't break the build if that module is missing.

For example, for a missing plugin I can provide an empty function and for a missing config just an empty object will do. This is necessary because I need intellisense to actually boot even in the face of errors. Once it's up and running we can offer diagnostics pointing out that something is broken / missing.

The Prettier plugin is a similar situation — I think it'd be useful for the majority of things to work even if something can't be found — though I could be convinced that for Prettier at least it doesn't matter as much. But at the very least this is pretty critical for Intellisense DX.

@philipp-spiess
Copy link
Member Author

For the purposes of Intellisense (and probably Prettier) I need to to know what kind of thing is being loaded so I can stub in a value that doesn't break the build if that module is missing.

@thecrypticace Ah this makes sense, what if we pass a third argument that is either "module" or "config" or so? 🤔

@philipp-spiess
Copy link
Member Author

Basically you'll eagerly create the context node, store functions that load the stylesheet, parse it, etc… and then replace the nodes / metadata of the context node when it's done loading.

Ahh good idea with the mutation! yep this makes sense and I think would make things a bit simpler 👍

@thecrypticace
Copy link
Contributor

For the purposes of Intellisense (and probably Prettier) I need to to know what kind of thing is being loaded so I can stub in a value that doesn't break the build if that module is missing.

@thecrypticace Ah this makes sense, what if we pass a third argument that is either "module" or "config" or so? 🤔

I think "plugin" and "config" should be the values but yep that would def work!

@philipp-spiess
Copy link
Member Author

@thecrypticace regarding this comment:

Caveats
One thing to note is that we previously relied on finding specific symbols in the input CSS to bail out of Tailwind processing completely. E.g. if a file does not contain a @tailwind or @apply directive, it can never be a Tailwind file.

Since we no longer have a string representation of the flattened CSS file, we can no longer do this check. However, the current implementation was already inconsistent with differences on the allowed symbol list between our clients. Ideally, Tailwind CSS should figure out wether a CSS file is a Tailwind CSS file. This, however, is left as an improvement for a future API since it goes hand-in-hand with our planned API changes for the core tailwindcss package

Is this a concern for Intellisense as well? i.e do you introspect the CSS file to determine if it's a likely Tailwind root? Might make it a higher priority to add this sooner rather than later. I guess nothing will break just now though since we still use postcss-import there for the time being.

@thecrypticace
Copy link
Contributor

Is this a concern for Intellisense as well? i.e do you introspect the CSS file to determine if it's a likely Tailwind root? Might make it a higher priority to add this sooner rather than later. I guess nothing will break just now though since we still use postcss-import there for the time being.

Yep it is but nothing will break because it uses postcss-import. Having a central spot for that logic would be nice but I don't think it's critical to have in core right now. Maybe can look at doing that in a followup PR?

packages/tailwindcss/src/at-import.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/at-import.ts Show resolved Hide resolved
packages/tailwindcss/src/at-import.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/compat/apply-compat-hooks.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/compat/apply-compat-hooks.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/at-import.test.ts Show resolved Hide resolved
packages/tailwindcss/src/at-import.test.ts Outdated Show resolved Hide resolved
packages/@tailwindcss-node/src/compile.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/at-import.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/at-import.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/at-import.ts Outdated Show resolved Hide resolved
@philipp-spiess philipp-spiess merged commit 7979474 into next Sep 23, 2024
3 checks passed
@philipp-spiess philipp-spiess deleted the feat/at-import-resolver branch September 23, 2024 15:05
thecrypticace added a commit to tailwindlabs/tailwindcss-intellisense that referenced this pull request Sep 24, 2024
…1058)

Related to tailwindlabs/tailwindcss#14446

We'll be handling `@import` resolution in core with the appropriate
hooks to ensure that all I/O is done outside of the core package. This
PR preps for that.
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.

3 participants