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

Generate JavaScript tokens #790

Merged
merged 6 commits into from
Jun 16, 2020
Merged

Generate JavaScript tokens #790

merged 6 commits into from
Jun 16, 2020

Conversation

tylersticka
Copy link
Member

@tylersticka tylersticka commented Jun 11, 2020

Overview

This is an alternative approach to #788. Instead of exposing Theo tokens directly via Rollup, the tokens are preprocessed and JavaScript versions are generated.

The drawback to this approach are that the import paths are a little longer and recompiling token changes is a bit slower.

But there are a few benefits:

  1. It appears to play nicely with TypeScript, and does not seem to require any separate declaration files or additions to our existing types.ds file.
  2. It allows you to import values directly instead of as a glob.
  3. In the future, this may invalidate our need for the Theo Webpack loader, which would further simplify our configuration.

Screenshots

Screen Shot 2020-06-11 at 1 48 48 PM

Testing

  1. Check out branch locally.
  2. Attempt to import a design token of your choosing using one of those documented in Storybook. (To avoid having to create a new file, try adding it to the Elastic Textarea script.)
  3. Run npm run build
  4. See if the token made it to dist/cloudfour-patterns.js.
  5. Run npm run lint and npm run type to make sure there are no errors after that change was made.

/CC @calebeby

@changeset-bot
Copy link

changeset-bot bot commented Jun 11, 2020

💥 No Changeset

Latest commit: 97554e5

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@tylersticka tylersticka marked this pull request as ready for review June 11, 2020 21:00
@tylersticka tylersticka requested a review from a team June 11, 2020 21:00
Copy link
Member

@spaceninja spaceninja left a comment

Choose a reason for hiding this comment

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

This failed for me. I added

import { textDark } from '../../design-tokens/generated/colors.js';
console.log(textDark);

to elastic-textarea.ts and then ran npm run build per your testing instructions.

It failed because colors.js doesn't exist. It looks like the build command doesn't run the preprocess command. I think we could just add preprocess to the build command like so:

npm run clean && npm run preprocess && run-p build:*

.theo/formats/stories.mdx.js Outdated Show resolved Hide resolved
@tylersticka
Copy link
Member Author

tylersticka commented Jun 15, 2020

@cloudfour/dev @calebeby My heart sank as I added preprocess to our build script and encountered a new error whenever I tried including one of the design token files:

error TS5055: Cannot write file '/Users/tylersticka/Repos/cloudfour.com-patterns/src/design-tokens/generated/colors.js' because it would overwrite input file.

I tried following the recommendations in this Stack Overflow thread, but they don't seem to work.

I'm beyond frustrated. So far, 100% of my experience with TypeScript has been resolving unhelpful errors. I don't know what else to try.

@calebeby
Copy link
Member

This looks like it is an error coming from dts-bundle-generator, if npm run type passes but the gulp buildScripts fails.

If that is the case, then I think we should find an alternative to dts-bundle-generator, since it has been causing a lot of problems

@spaceninja
Copy link
Member

spaceninja commented Jun 15, 2020

@calebeby as a way to keep Tyler moving forward with this PR, could we temporarily comment out the buildTypes command?

If not, is there something we could do to help Tyler get this unblocked? I'm unfamiliar with what dts-bundle-generator is doing, so I'm not confident in finding a replacement.

Basically, I think we need to prioritize getting this PR unblocked. That could mean either we fix the root problem if it's quick, or we add a short-term workaround and then file an issue to fix the problem the right way as a separate effort.

@calebeby
Copy link
Member

calebeby commented Jun 15, 2020

Since we have continuous publishing to npm, I think we shouldn't merge this PR to v-next with buildTypes commented out, because that means that a release could be triggered to npm with types missing, which should be a breaking change.

@tylersticka If you want I can make the changes in another PR today to switch out dts-bundle-generator

@tylersticka
Copy link
Member Author

@calebeby That would be great! Thank you for your help.

@tylersticka tylersticka requested review from a team and spaceninja June 15, 2020 22:11
@tylersticka
Copy link
Member Author

Feedback addressed and updated with @calebeby's TypeScript improvements. Appears to work on my end once testing steps are followed.

Copy link
Member

@spaceninja spaceninja left a comment

Choose a reason for hiding this comment

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

Works great!

@tylersticka tylersticka merged commit 2e00bb2 into v-next Jun 16, 2020
@tylersticka tylersticka deleted the feature/tokens-js branch June 16, 2020 17:58
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.

Figure out how to consume design tokens in client-side JS
3 participants