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

CSS Plugin default transform should expose the same API and params with the custom one #145

Closed
radum opened this issue Nov 6, 2023 · 10 comments
Labels
bug Something isn't working

Comments

@radum
Copy link

radum commented Nov 6, 2023

Hello,

Thank you so much for Cobalt its been a life saving module so far :)

I have an issue with custom transforms when using the CSS Plugin. Since this commit there have been some changes on how the internals work that could break or not bring the same flexibility for custom build transforms.

For example the current API for transform transform(token, mode) {} only has access to token and mode but if we want to reproduce the capabilities of the existing default transform we can't unless we reimplement some of the functionality ourselves.

For example looking at the typography one

case 'typography': {
      const {value, originalVal} = getMode(token, mode);
      if (typeof originalVal === 'string') {
        return varRef(originalVal, tokens, generateName);
      }
      const output: Record<string, string> = {};
      for (const [k, v] of Object.entries(value)) {
        const formatter = k === 'fontFamily' ? transformFontFamily : (val: any): string => String(val);
        output[kebabinate(k)] = isAlias((originalVal as any)[k] as any) ? varRef((originalVal as any)[k], tokens, generateName) : formatter(v as any);
      }
      return output;
    }
    default: {
      throw new Error(`No transformer defined for $type: ${(token as any).$type} tokens`);
    }
  }

varRef

This needs new params that are not exposed to the transform() params. For example tokens: ParsedToken[], generateName: ReturnType<typeof makeNameGenerator> are needed and we can't make use of it.

We can get the list of parsed tokens via the core parser but that means we need to parse them again when they are already available.

And that is just one example.

I think the capabilities of the default transformer should be exposed in a reusable way to make sure we can reimplement the needed functionality.

There are more items to talk about this and I am wiling to help do that but just wanted to get your thoughts on it first.

@drwpow
Copy link
Collaborator

drwpow commented Nov 6, 2023

Thanks for this feedback! And just to clarify, does the config() hook let you get access to the generateName() function as intended? Or is there something else needed? edit: had a brainfart; confused the CSS plugin options with the internal plugin options (i.e. you developing your own plugin). But my edited question would be: could you speak more why you need the generateName() option during transform?

For the tokens: ParsedToken[] bit could you speak a bit more about why you are transforming token A but need access to token B? Would want to understand why you’d need access to all the parsed tokens first before digging into a fix and/or workaround.

@drwpow
Copy link
Collaborator

drwpow commented Nov 6, 2023

Ah sorry—yes there was an unintentional breaking change I’ll be releasing a patch for. I misinterpreted this as a need and not a bug. Will be releasing a patch to plugin-css and plugin-sass later today.

@drwpow
Copy link
Collaborator

drwpow commented Nov 6, 2023

Just published a patch version for plugin-css and plugin-sass. Let me know if this fixes your issue (I was able to recreate what you’re talking about in another project, and was able to fix some obvious breakages, but wanted to confirm I captured everything in your usecase as well) 🙏!

@drwpow drwpow added the bug Something isn't working label Nov 6, 2023
@radum
Copy link
Author

radum commented Nov 7, 2023

Hey thank you very much for you quick reply and fix.

Let me see if I can explain better. The available CSS plugin has a default transformer export function defaultTransformer.

In the same time it exposes an API for us to provide a custom transformer in case our tokens look different or we might want for example to convert PX to REM or whatever.

If want to replicate the default transformer logic with minor changes like it is in your code here:

case 'typography': {
      const {value, originalVal} = getMode(token, mode);
      if (typeof originalVal === 'string') {
        return varRef(originalVal, {prefix, tokens, generateName});
      }
      const output: Record<string, string> = {};
      for (const [k, v] of Object.entries(value)) {
        const formatter = k === 'fontFamily' ? transformFontFamily : (val: any): string => String(val);
        output[kebabinate(k)] = isAlias((originalVal as any)[k] as any) ? varRef((originalVal as any)[k], {prefix, tokens, generateName}) : formatter(v as any);
      }
      return output;
    }

I should be able to "copy paste" that code import the missing references like fn varRef and apply my customisations.

Basically my config looks like this

plugins: [
		pluginCSS({
			filename: './_tokens.scss',
			transform(token) {
				if (token.$type === 'typography') return transformTypography(token, result.tokens);
			}
		})
	]

That transformTypography() func should handle my use case. But I don't want to loose the functionality the baked in one has (like passing the alias when one is available and not just the raw value). To do that I can copy the default one and refactor it.

Before you patched it recently one couldn't do that because some of those internal deps were not exposed. In your latest patch I see you removed them and now I think it should be fine but in general I think that the capabilities of the default transformer should be made available to custom ones we might build.

In my particular example for typography I wanted to create fluid typography with clamp, so I added an extension key to my json and I wanted to check for its existence and then convert the fontSize to something else but keep the rest.

This is a perfect example for that custom transform that can be passed to the css plugin.

Hope it makes sense now.

@radum
Copy link
Author

radum commented Nov 7, 2023

What I ended up doing btw was something like this

import { isAlias } from '@cobalt-ui/utils';
import { varRef, transformFontFamily, defaultNameGenerator } from '@cobalt-ui/plugin-css';

pluginCSS({
			filename: './_tokens.scss',
			transform(token) {
				// As there is no way to pass the TokenResult[] to some of the internals we have to parse them again here.
				const { errors, warnings, result } = co.parse(typographyTokens);

				if (errors) console.error(errors);

				// This is a reimplementation of `typography` case transform from the default transform in the CSS plugin.
				if (token.$type === 'typography') return transformTypography(token, result.tokens);
			}
		}),

And then that func was:

export function transformTypography(token, parsedTypographyTokens) {
	const value = token.$value;
	const originalVal = token._original.$value;

	const fontSize = token.$value.fontSize;
	const tokens = parsedTypographyTokens;
	const generateName = (variableId) => {
		const DASH_PREFIX_RE = /^-+/;
		const name = defaultNameGenerator(variableId, '');
		return `--${name.replace(DASH_PREFIX_RE, '')}`;
	};

	if (typeof originalVal === 'string') {
		return varRef(originalVal, tokens, generateName);
	}

	// Check if its a fluid typography type or not and convert the font size to clamp or if its a normal font size use REM
	let newFontSize;
	if (token?.$extensions['org.toyota.bz4x']?.$type === 'typography-fluid') {
		const minFontSize = token.$value.minFontSize;
		newFontSize = clampGenerator(minFontSize, fontSize, 'px');
	} else {
		newFontSize = convertFontSizeToRem(fontSize);
	}

	const newValue = {
		...value,
		fontSize: newFontSize
	};
	delete newValue.minFontSize;

	const output = {};
	for (const [k, v] of Object.entries(newValue)) {
		const formatter = k === 'fontFamily' ? transformFontFamily : (val) => String(val);
		output[kebabinate(k)] = isAlias(originalVal[k]) && k !== 'fontSize' ? varRef(originalVal[k], tokens, generateName) : formatter(v);
	}

	return output;
}

So replicating the internals was more of a hack :)

With the latest patch I think I can simplify it even more. I will check.

@radum
Copy link
Author

radum commented Nov 7, 2023

So I managed to simplify but varRef now as I am not passing generateName will not get back with the correct alias:

image

Also the log is full of Tried to reference variable with id: text.family.1, no token found as I am not passing the tokens :)

@dev-nicolaos
Copy link
Contributor

dev-nicolaos commented Nov 9, 2023

Hey @radum, sorry for any churn I caused in the latest minor version.

i think maintaining a large public API is going make development on this plugin difficult. There's a lot of things that get exported from the plugin so that they can be used in other internal packages (mainly plugin-sass) but aren't publicly documented (varRef is one example). @drwpow It would be helpful to clarify if these functions are part of the public API (consumers can import and use them safely) or if they're considered an implementation detail and subject to breaking changes without notice.

I tried to address this for the new exports I added in #131: defaultNameGenerator is part of the public API and is documented in the docs while makeNameGenerator is renamed when being exported to _INTERNAL_makeNameGenerator and not documented since its not meant to be part of the public API. But I didn't want to make an assumption about the existing undocumented exports and so didn't address them.

@radum
Copy link
Author

radum commented Nov 9, 2023

@dev-nicolaos Thats alright.

I agree with you, but :) in the same time to make it extensible we need to be able to replicate internal functionality. Some parts are critical and I think varRef is one of those.

I wouldn't imagine how we would build custom plugins or transforms without having varRef for example. So I think we need map out what are core pieces of functionality that are required to make a plugin or to build a transform and those should be publicly exposed.

@drwpow
Copy link
Collaborator

drwpow commented Dec 4, 2024

So good news! This is now supported in the 2.0 Plugin API (docs), which is designed around the idea that plugins need to be able to share work more efficiently.

Even the idea of varRef() was codified into something called a “localID”, so when the CSS plugin generates tokens for CSS, the global ID will be color.blue.600 but the local ID will be --color-blue-600.

Upgrading to Terrazzo shouldn’t be too hard; the plugins follow the same format, but now transform() is used for creating the values, and you should move work into there if possible. See the migration guide for more info.

@drwpow drwpow closed this as completed Dec 4, 2024
@radum
Copy link
Author

radum commented Dec 9, 2024

This is amazing @drwpow

I will have a look and convert our local implementation and provide feedback. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants