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

feat(build-file): add escape return if properties is empty #494

Merged
merged 4 commits into from
Jan 23, 2021

Conversation

davidyeiser
Copy link
Contributor

@davidyeiser davidyeiser commented Dec 2, 2020

Context

I have a setup where tokens are generated for multiple apps. There is a global "app" for all the shared tokens and then each app has its own unique tokens. The transformations for the platforms are run for each app.

Problem

The problem I'm running into is for example the global app has tokens for colors and gradients that are output with custom formats but then for say "App A" when the transformations are run and App A doesn't have any color or gradient tokens the custom formatter is run for each with no data which causes two things happen:

  1. For the color tokens, an empty file is created.
  2. For the gradient tokens, an error occurs because of the template logic trying to operate on undefined variables.

Solution

The solution I came up with was to add a check in lib/buildFile.js after filteredProperties() is run to see if the resulting allProperties array is empty. If it's empty then the function returns null before the custom format file is built.

I've tested it in my Style Dictionary project and it works as intended. The build process produces only the final files for the tokens present. However, there may be large issues here that I'm not aware of; or perhaps it's more of an issue with my setup than the need for extra code in the core?

I'm happy to make further changes, etc. if required.

Thanks!

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@davidyeiser
Copy link
Contributor Author

Ah, I see the tests now, let me address these (first timer here haha)

@davidyeiser davidyeiser changed the title feat(buildFile): add escape return if allProperties is empty feat(build-file): add escape return if properties is empty Dec 2, 2020
@davidyeiser
Copy link
Contributor Author

The should work with a filter test in buildFiles.test.js was failing — I think because the dictionary passed to the test didn't have an allProperties array. I've changed the conditional to check filteredProperties.properties instead. (The check for an empty object is a little messy, I could use lodash’s _.isEmpty() method instead if that was desired.)

@chazzmoney
Copy link
Collaborator

Thanks @davidyeiser - this was an edge case we did not consider.

Would you be able to add code to output a message to the user? Just so they know the file was not created and the reason why.

You can use console.log and chalk as needed.

Thank you!

@davidyeiser
Copy link
Contributor Author

Yep, no problem. I'll plan on having it completed by the end of the week.

@davidyeiser
Copy link
Contributor Author

I added a console message. Feel free to change the wording, style, etc. if you see fit. Currently it has a warning feel to it, but not too strong.

I also added a corresponding test, however, I've never written a test before and I was just basing it off of the others around it. So I can't say with 100% certainty that it's doing its job but it at least outputs the warning in the test.

@davidyeiser
Copy link
Contributor Author

(Not sure why the Node 8 test is failing)

@dbanksdesign
Copy link
Member

Ah don't worry about node 8, we are removing support for it since it is not supported anymore. I'm going to switch the branch to the 3.0 branch as that will be the next formal release, and that will fix the travis build error because we removed node 8 from our travis file.

@chazzmoney chazzmoney changed the base branch from main to 3.0 January 23, 2021 00:08
@chazzmoney
Copy link
Collaborator

LGTM! :shipit:

Thanks @davidyeiser !

Copy link
Member

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! :shipit: Thank you so much for this addition!

@chazzmoney chazzmoney merged commit 8945c46 into amzn:3.0 Jan 23, 2021
dbanksdesign pushed a commit that referenced this pull request Jan 26, 2021
Note: This is a breaking change as it changes the behavior of SD from 2.x
@Razinsky
Copy link

Hi there! Can we put this feature under a flag please? In my use case, I take advantage of the build process to create an index.ts file that imports all the generated files to create a theme object. Of course, this index.ts file does not need any token but that is on purpose.

@dbanksdesign
Copy link
Member

@Razinsky interesting. Can you go into a bit more detail about your setup? So you use Style Dictionary to build a file without any tokens in it? Could you build that file outside Style Dictionary instead?

@Razinsky
Copy link

Razinsky commented Apr 1, 2021

Hi @dbanksdesign ! Sure, I use Style Dictionary to build for Typescript. Here's the build structure:

{platform}/
└─ {brand}/
   └─ {mode}/
       ├─ colors.ts
       ├─ object-styles.ts
       ├─ spacing.ts
       ├─ typography.ts
       └─ index.ts  // the file without tokens

And here's how the index.ts looks like:

import { default as colors, ColorKeys, Colors } from './colors';
import { default as styles, Styles } from './object-styles';
import { default as spacing, Spacing } from './spacing';
import { default as typography, Typography } from './typography';

const designTokens = {
  colors,
  spacing,
  styles,
  typography,
} as const;

type DesignTokens = typeof designTokens;

export { Colors, ColorKeys, DesignTokens, Spacing, Styles, Typography };

export default designTokens;

I know I could do it after the SD build but I like the convenience of using the build to create this file. And since I also build for CSS, Android and iOS, I would have to recreate the same kind of logic that triggers the various builds but just for building extra files...

Obviously, if we decide not to put this feature under a flag, I'll have to build it outside anyway... All in all, for me it was a convenience more than anything. Just tell me if it makes sense :)

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

Successfully merging this pull request may close these issues.

4 participants