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

Allow empty groups? #206

Closed
james-nash opened this issue Mar 4, 2024 · 6 comments · Fixed by #208
Closed

Allow empty groups? #206

james-nash opened this issue Mar 4, 2024 · 6 comments · Fixed by #208
Labels
bug Something isn't working

Comments

@james-nash
Copy link
Contributor

I've noticed that Cobalt errors when it encounters an empty group in a DTCG file. In my current setup, which uses Tokens Brücke to export DTCG from our Figma library's variables, I end up with an empty group. (FWIW, the reason we have an empty group is because our design system is still a work in progress, and we're scaffolding out groups before adding actual tokens into them.)

I've worked around this with a simple script that removes the offending group from the DTCG file, before it gets passed into Cobalt. However, I think it would be helpful if Cobalt was more permissive and just ignored empty groups rather than throwing an error. (or else this was downgraded to a warning)

It would simplify my setup a little. But I suspect it's a situation others might also encounter, so it could help others too.

FWIW, the DTCG spec doesn't say anything about empty groups, so Cobalt's approach is a valid interpretation. However, I'll raise an issue in the DTCG repo later to suggest amending the spec to be more specific about whether (or not?) empty groups are allowed.

@drwpow
Copy link
Collaborator

drwpow commented Mar 4, 2024

However, I think it would be helpful if Cobalt was more permissive and just ignored empty groups rather than throwing an error. (or else this was downgraded to a warning)

Yeah I think just raising a warning and ignoring it is the right call. I think people should be aware they have empty groups in their tokens.json. But you’re right—it shouldn’t prevent generation. I don’t think we should warn for groups that, say, have $description or $extensions but not tokens; only for truly empty groups ({}).

I never made an intentional decision to err on empty groups; I just really never thought to test for it. I could release a patch for this fairly quickly.

@drwpow drwpow added the bug Something isn't working label Mar 4, 2024
@drwpow
Copy link
Collaborator

drwpow commented Mar 4, 2024

I never made an intentional decision to err on empty groups; I just really never thought to test for it. I could release a patch for this fairly quickly.

Oops! I lied; I completely forgot we do err on empty groups. Let me change my answer and say “though Cobalt does err now, I do not feel strongly about it at all; and it was likely more a decision to just “not deal with it” and force the user to change their schema.” I don’t think downgrading to a warning is a significant “breaking change” that would disrupt many people.

@c1rrus
Copy link

c1rrus commented Mar 4, 2024

Sweet! Thanks for the quick response. If it helps, I could contribute a PR for this.

@c1rrus
Copy link

c1rrus commented Mar 4, 2024

(btw, @james-nash is me too. I have separate GitHub accounts for work and personal stuff)

@drwpow
Copy link
Collaborator

drwpow commented Mar 4, 2024

Sweet! Thanks for the quick response. If it helps, I could contribute a PR for this.

Would love a PR for this! Should be as simple as changing this line in core. Also selfishly would love for the great James Nash’s name to be a contributor to this project 🙂

(btw, @james-nash is me too. I have separate GitHub accounts for work and personal stuff)

Aha! I knew it! No wonder I had never seen you both in the same room before 😉

@c1rrus
Copy link

c1rrus commented Mar 4, 2024

Cool, I'll have a go at making a PR tomorrow.

Aha! I knew it! No wonder I had never seen you both in the same room before 😉

"Never cross the streams" It's a miracle that the universe didn't implode! 🤪

james-nash added a commit to james-nash/cobalt-ui that referenced this issue Mar 5, 2024
james-nash added a commit to james-nash/cobalt-ui that referenced this issue Mar 5, 2024
drwpow pushed a commit that referenced this issue Mar 5, 2024
* Fix empty groups causing parsing errors (#206)

* Added test for empty groups (#205)

* Add changeset (#206)
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

Successfully merging a pull request may close this issue.

3 participants