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

Cannot override built in transforms #1206

Closed
okadurin opened this issue May 16, 2024 · 1 comment
Closed

Cannot override built in transforms #1206

okadurin opened this issue May 16, 2024 · 1 comment

Comments

@okadurin
Copy link

okadurin commented May 16, 2024

In v4 if trying to register a transform with the name which is already registered internally, the custom transform function will never be called. Instead the internal one will be called. In v3 it worked. Either because name/camel was not defined internally or because the library allowed overriding transforms, not sure. Here is example of the code. Transform group is defined at the config object.

  StyleDictionary.registerTransform({
    name: 'name/camel',
    type: 'name',
    transform(token, options) {
       //...
    },
  });

  StyleDictionary.registerTransformGroup({
    name: 'custom/ios',
    transforms: ['name/camel'],
  });

If renaming name/camel it starts working.


This could be a documentation update (if any) or the code could be updated so that if the user registers a transformer with the internal name, then the library allows to override it. Maybe it's not even internal names. Maybe it just ignores registerTransform for the same name after it was registered once. I did not check that. For me I just renamed the transform and the code began to work.

@jorenbroekema
Copy link
Collaborator

jorenbroekema commented May 20, 2024

Yeah this is probably a regression of the v4 restructure of the StyleDictionary object and how hooks are now registered.

I'm guessing the desired behavior when a hook under name X already exists is:

  • throw a warning to the user that a hook is being overwritten (not a thing in v3 nor v4, so this is an enhancement)
  • override the already registered hook, replace with the new one (fixes the regression bug of v4)

https://github.com/amzn/style-dictionary/blob/v4/lib/Register.js#L100
The code here is what's wrong, it's using deepmerge for registering hooks but merging is not appropriate here is the desired behavior is overriding, instead it should be:

target.hooks = {
  ...target.hooks,
  transforms: {
    ...target.hooks.transforms,
    [name]: {
      type,
      filter,
      transitive: !!transitive,
      transform: transformFn,
    },
  },
});

And repeat that pattern for the other 7 hooks as well within that Register class.

PRs welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants
@okadurin @jorenbroekema and others