-
Notifications
You must be signed in to change notification settings - Fork 63
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
Token name - reserved words #61
Comments
Hmm. We use |
The purpose of reserved words is to keep users from colliding with words that have a specific meaning to the language. Is there a language taxonomy for this project? Like @jina pointed out, people use I am curious why you picked |
I've seen |
oh interesting. for some reason that's not been an issue for our ios tokens yet. I'll bring it up with our team just in case. :) |
There's a bunch of property names that have a special meaning in our spec, so I believe these necessarily need to be reserved words: It might be a good idea to have a section somewhere that lists them all for convenience. It's likely that future spec iterations will add more (we've already been talking about things like If we do, we may unnecessarily limit what people can name their tokens and groups (e.g. it seems If we don't, we may may find it hard to add more properties in future spec versions. So, how about this as an alternative: What if all spec properties started with a certain symbol. Taking a bit of inspiration from JSON schema, we could use a So, {
"$description": "The description of this group",
"token name": {
"$value": "100px",
"$type": "dimension"
},
"another token": {
"$value": "#abcdef",
"$type": "color"
}
} Pros:
Cons.
Thoughts? |
Turns out the way we are exporting our tokens and for the code gen we have, this isn't an issue for us at work. Phew! |
This could be avoided, as you suggested, with some prefix like $, so only Another way would be to add just one special keyword {
"description": "The description of this group",
"value": "just a custom property"
"tokens": {
"any word can be a token name": {
"value": "100px",
"type": "dimension"
},
"value": {
"value": "#abcdef",
"type": "color",
"description": "this token is used for item value"
},
"description": {
"value": "#ffffff",
"type": "color",
"description": "this token is used for description texts"
}
}
} Root level tokens would also not need any exception if we threat {
"token name": {
"value": "100px",
"type": "dimension"
},
"value": {
"value": "#abcdef",
"type": "color"
},
"description": {
"value": "#ffffff",
"type": "color"
}
} I wrote a maybe too big example in #55 (comment) |
I'm not keen on the I think documenting our current reserved words and having a rule in the spec that a token cannot be named:
is reasonable. If we need to add new reserved keys in the future we can circulate those in the community to gauge the level of collision. It does mean future versions of the spec that add new reserved keys would be breaking changes, which is a bummer, but my gut says we're over engineering this if we're trying to avoid those collisions already. |
I agree with @kevinmpowell here 👍 |
Additional Prefixes are also solving the problem and the only con I see is ease of access if prefix is Current proposal with 4 reserved keywords is ok today, but I am pretty sure that it will not be enough as the project matures. Then the choice will be either breaking change, which is IMO a huge deal in projects like this, or some hacky solutions where everything is for example added to Given the impact of these decisions, I would like to ask you for a bit more discussion, both around possible prefixes, additional node, and impact of breaking change if a new reserved word is added later. |
I've been working on building tooling for tokens at Microsoft for a couple of years now, and one of my biggest gripes with Style Dictionary today is how they used I'm very much in favor of prefixed keywords like const tokens = { red: { $value: "#ff0000ff" } }
console.log(tokens.red.$value) |
Funny how we all just accepted that So what would actually be downside of this except maybe personal preference? My personal preference is still the idea of having But let's not make decisions based on personal preferences. And let's not just try to confirm that initial idea with reserved words is good. I think the modern term for it would be confirmation bias ;) |
Reserved words are definitely good. Just less common words are better,
hence the $.
…On Thu, Dec 2, 2021 at 7:51 PM Ivan Maksimovic ***@***.***> wrote:
Funny how we all just accepted that $value has access issue without even
checking. @TravisSpomer <https://github.com/TravisSpomer> is correct. We
would not need to access things like token['$value'] as token.$value is
perfectly valid syntax.
So what would actually be downside of this except maybe personal
preference? My personal preference is still the idea of having tokens or
maybe $tokens node for tokens because of groups and group properties.
But let's not make decisions based on personal preferences. And let's not
just try to confirm that initial idea with reserved words is good. I think
the modern term for it would be confirmation bias ;)
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#61 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEKS36GCR4IYPGB3LXLU7E3UO56HHANCNFSM5DXWWQCA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
You're quite right @TravisSpomer, thanks for pointing out my mistake about things in JS not being allowed to begin with I do agree this topic needs further consideration and debate. @ivnmaksimovic's suggestion of nesting a group's children under a {
"group-1": {
// "special" group properties
"description": "Only the coolest tokens get to be in this group!",
"type": "dimension",
// this group's items:
"tokens": {
"token-1": { "value": "2rem" },
"token-2": { "value": "99px" },
"nested-group-1": {
"description": "A nested group",
"tokens": {
"deep-token-1": { "value": "1rem" },
// ...
}
},
// ...
}
},
// So far, so good. But when your group doesn't use any special properties
// you still need put the items inside a tokens prop.
"group-2": {
"tokens": {
"token-A": { "value": "#ff0000", "type": "color" },
"token-B": { "value": "#00ff00", "type": "color" },
"nested-group-A": {
"tokens": {
"deep-token-A": { "value": "#00ff00", "type": "color" },
// ...
}
},
// ...
}
}
} What if we flip the idea though? Rather than nesting the group's items, we nest the special properties instead. Let's imagine we have a special {
"group-1": {
// "special" group properties
"metadata": {
"description": "Only the coolest tokens get to be in this group!",
"type": "dimension"
},
// this group's items (now at the top level):
"token-1": { "value": "2rem" },
"token-2": { "value": "99px" },
"nested-group-1": {
"metadata": {
"description": "A nested group"
},
"deep-token-1": { "value": "1rem" },
// ...
}
// ...
},
// Groups that don't use any special properties remain the same as they are
// in the current spec draft
"group-2": {
"token-A": { "value": "#ff0000", "type": "color" },
"token-B": { "value": "#00ff00", "type": "color" },
"nested-group-A": {
"deep-token-A": { "value": "#00ff00", "type": "color" },
// ...
}
// ...
}
} We'd still have the same benefits:
What do you think? |
I certainly prefer the idea of nesting the special properties. That seems to align with Style Dictionary's CTI convention out of the box. This allows you to filter on the special properties without needing to loop through the properties. I like grouping them in something like metadata that can be ignored by default during processing. You can then easily output something like .format where the output is shallow and the metadata is excluded and .raw.format where the metadata is included. |
I support @c1rrus' proposal to use a
But this also means we wouldn't need to expand the list of reserved words beyond |
I like having |
I also like keeping things simple and having +1 to reserve |
With the changes in PR #89 only Since The reason |
Ahoy, late to the chat. I'm trying to figure out what counts towards "non-token name" properties when avoiding reserved words. |
FYI: There's been some good debate over on PR #89 which I had started with the intent of updating the spec to use the The main concern is the inconsistency between groups (where "special" properties are nested under The format editors discussed this at our last meeting and agreed this needs some more thought before we decide how to proceed. In light of that, I'm starting to wonder if it's worth revisiting the
What do you think? Is this perhaps a better alternative to the |
I'm all for using a prefix instead of |
Prefixes also improve human readability.
…On Fri, 14 Jan, 2022, 12:28 am Matthew Ström, ***@***.***> wrote:
I'm all for using a prefix instead of metadata! I have a very slight and
personal aesthetic preference for using _value instead of $value because
the $ looks like an S. Very slight and very personal :)
—
Reply to this email directly, view it on GitHub
<#61 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEKS36AISKXZYDK7HHE3DDLUV4OHHANCNFSM5DXWWQCA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you commented.Message ID:
***@***.***>
|
I don't see any flows with reserved prefix approach and I agree that it improves human readability. Unlike other proposals where user would have to learn more about the format, prefixes would serve as a hint that those are special properties. I also agree that |
|
Does anyone have objections to going down the prefix route? If not, I think we should press ahead and update the spec draft accordingly. All that remains is to choose a prefix. I don't have a strong preference myself. As @TravisSpomer noted, JSON-LD on the other hand uses And then there's the How about we do a vote? please react with the emoji that corresponds to your preferred prefix:
|
@c1rrus great idea on the poll! Let's close this puppy out 😄 |
If it is matters at all, quite a few languages like C++, C#, Java, Python allows variable name to start only with a-z, A-Z and "_". (result of quick search. I haven't really checked that). |
Editor decision 1/25/22: moving forward with |
Are there any reserved words that should not be used in token names?
Examples: 'default', 'int'
Will these (and others) cause problems when converted to various languages and platforms?
Resolution: #61 (comment)
The text was updated successfully, but these errors were encountered: