Extract more backwards compatibility logic to compatibility layer #14365
Conversation
…nditions for applying hooks
This is handled by the CSS function already.
| loadPlugin, | ||
| configPaths, | ||
| loadConfig, | ||
| globs, |
There was a problem hiding this comment.
Some thoughts:
- This should definitely take an object as a param
- I think this object should (eventually) have a name of some kind. Especially given that it basically represents structured data / knowledge about the input CSS. Not immediately but maybe a
Tailwindobject or something that metadata attached to it about the current CSS? I think this goes to the larger API design stuff we were talking about at one point. (idk could even call itCSSFileor something but that doesn't "feel" quite right) - I think we should do the AST walk and validation stuff for
@pluginand@configin this function too (maybe). It already takesastas an argument and you could eliminatepluginPathsandconfigPathsoptions. The downside here being there's an extra AST walk but we're already doing multiple walks. If we're able to optimize those maybe on extra walk won't be as big of a deal.
There was a problem hiding this comment.
Yeah moving the @plugin and @config extraction into the compat hooks would be nice since currently that information is only used in that later.
Later, when there is a CSS @plugin API, we could leave @plugin 'path' untouched in the core implementation and can then separate between the core v4 implementation and the compat one more cleanly as well.
philipp-spiess
left a comment
philipp-spiess
left a comment
There was a problem hiding this comment.
I like this! Makes me wander of we should have a build/config option of v4 with the compat hooks removed already for people that wan't to live on the bleeding edge? :D
|
|
||
| resolveThemeValue(path: string, defaultValue?: string) { | ||
| return resolveThemeValue(theme, path, defaultValue) | ||
| resolveThemeValue(path: `${ThemeKey}` | `${ThemeKey}${string}`) { |
There was a problem hiding this comment.
Should we add a comment here that this can be overwritten later by the compatibility implementation? Otherwise, from looking at a stack trace, people might be confused as to why stuff doesn't match up at runtime.
| loadPlugin, | ||
| configPaths, | ||
| loadConfig, | ||
| globs, |
There was a problem hiding this comment.
Yeah moving the @plugin and @config extraction into the compat hooks would be nice since currently that information is only used in that later.
Later, when there is a CSS @plugin API, we could leave @plugin 'path' untouched in the core implementation and can then separate between the core v4 implementation and the compat one more cleanly as well.
The `--color-red-500` part was already handled before. This now only
handles legacy `theme()` lookup like `them('colors.red.500')`.
I still like that there is a "compat" folder because even if we never delete it, it would still be nice to have a clean distinction between old and new. Especially if we can make compat related things conditional such that you don't have to pay this cost if you only use the modern way. I think this will especially be helpful for bug reports, to know where to put the logic (old compat or new). |
|
Pushed a few small improvements |
This PR builds on top of #14365 and adds a few more changes we discussed during a sync on the latter PR: - We now split `plugin-api.ts` into two files and moved it into `compat/`. One file is now defining the comat plugin API only where as another file deals with the compat hook. - The walk for `@plugin` and `@config` is now happening inside the compat hook. The one remaining work item is to change the `loadPlugin` and `loadConfig` APIs to a more unified `resolveModule` one that does not care on what we try to load it for. I suggest we should make this change at the same time we start working on finalizing the `tailwindcss` APIs, since a lot of things will have to be rethought then anyways.
I noticed a lot more backwards compatibility concerns had started leaking into core, especially around the
themefunction, so did a bit of work to try and pull that stuff out and into the compatibility layer.Now the core version of
themeonly handles CSS variables (like--color-red-500) and has no knowledge of the dot notation or how to upgrade it. Instead, we unconditionally override that function in the compatibility layer with a light version that does know how to do the dot notation upgrade, and override that again with the very heavy/slow version that handles JS config objects only if plugins/JS configs are actually used.I've also renamed
registerPluginstoapplyCompatibilityHooksbecause the name was definitely a bit out of date given how much work it's doing now, and now call it unconditionally from core, leaving that function to do any conditional optimizations itself internally.Next steps I think would be to split up
plugin-api.tsa bit and maybe makeapplyCompatibilityHooksits own file, and move both of those files into thecompatfolder so everything is truly isolated there.My goal with this stuff is that if/when we ever decide to drop backwards compatibility with these features in the future (maybe v5), that all we have to do is delete the one line of code that calls
applyCompatibilityHooksinindex.ts, and delete thecompatfolder and we're done. I could be convinced that this isn't a worthwhile goal if we feel it's making the codebase needlessly complex, so open to that discussion as well.