build: add Chakra CLI #12034

TylerAPfledderer · 2024-01-27T18:49:53Z

Description

This PR adds back in the Chakra CLI to generate typing based on the custom theme configuration. This is helpful for IDE auto-completion to be able to recognize the correct tokens.

Adds scripts to both run the command once, and to have a watcher to update changes on the go.
Runs the postinstall script to generate the theme typing once yarn install has run

Related Issue

N/A

netlify · 2024-01-27T18:49:58Z

✅ Deploy Preview for ethereumorg ready!

Name	Link
🔨 Latest commit	`ba4f3d1`
🔍 Latest deploy log	https://app.netlify.com/sites/ethereumorg/deploys/66255e628611d80008ada76d
😎 Deploy Preview	https://deploy-preview-12034--ethereumorg.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

TylerAPfledderer

A couple of thoughts...

TylerAPfledderer · 2024-01-27T18:50:46Z

package.json

@@ -3,7 +3,8 @@
  "version": "8.0.0",
  "private": true,
  "scripts": {
-    "dev": "next dev",
+    "dev": "npx concurrently \"yarn next dev\" \"yarn theme:watch\"",


This is very much a personal preference. concurrently is not added to the project as a dev dependency, but run remotely. The only requirement that would be prompted in the console is for the user to install it to their local machine. (This package is used in Chakra projects)

This command ensures that the dev environment and the Chakra CLI run in parallel without the user needing to remember an extra step.

Can certainly revert this if this is too much.

Agreed, this is more of a personal preference. In that sense, I'd prefer to keep the dev script as clean as possible.

However, I do see the benefit of running the Chakra script in parallel. We could add a comment in the readme or docs to let devs know how this works.

cc @wackerow @nhsz @corwintines

Agree that this should be opt-in with another script, not default npm dev behavior, which should follow the standard. This could also bring some performance issues for some users, ram usage highly increased, etc, so should be optional.

I have reverted the dev script.

Would you want some comment added in this PR?

TylerAPfledderer · 2024-01-27T18:51:05Z

package.json

+    "theme": "chakra-cli tokens ./src/@chakra-ui/theme.ts --strict-token-types --strict-component-types",
+    "theme:watch": "chakra-cli tokens ./src/@chakra-ui/theme.ts --strict-token-types --strict-component-types --watch"


The strict-token-types and strict-component-types flags are used to constrain the typing, so that a combination of the string and {} primitives are not part of the type unions.

At the moment, this can aid in auto-completion. When talking with a member of the Chakra team, we recognized two issues (one kinda related):

If we have a component that is taking is props that are typed with a shape from Chakra (i.e. StackProps) and you spread them into a component (i.e. Stack), you lose typing if you also directly add props to the component

<Stack color="blue.500" // Typing lost here {...props} />

I would expect that with strict typing of the themes that TypeScript would throw an error if a typo or invalid token exists. This unfortunately does not seem to be the case.

These flags have now been removed. More details in a later comment with additional findings.

TylerAPfledderer · 2024-01-27T18:51:22Z

tsconfig.json

@@ -1,6 +1,6 @@
 {
  "compilerOptions": {
-    "target": "es5",
+    "target": "ESNext",


Otherwise an error throws with es5:
Transforming object literal extensions to the configured target environment ("es5") is not supported yet

ES6 should also work here

Won't this affect our browser support? if not sure, I'd recommend doing this in a separate PR to run tests accordingly.

@pettinarip According to the CanIUse site, ES6 shows 98% support.

https://caniuse.com/?search=ECMASCRIPT

I failed to clarify that this errors throws on instances of using cssVar() in component themes and attempting to compile dynamic keys... say:

[$colorBg.variable]: 'some value'

which is an ES6 only feature. Not sure if this is a Chakra CLI issue (doing testing in the Chakra package is not recreating the error), and really a typescript issue within this project.

I would point out that even the TypeScript docs also suggest es6 for the same reason as I mentioned above.

TylerAPfledderer · 2024-01-27T19:32:01Z

On initial failed build through Netlify, I believe this is exposing an inconsistency.

When yarn lint this only does checks with eslint. This does not account for TypeScript. Waiting for a build to run via GH CI and Netlify seems slow for Dev Experience.

I would consider a script to run tsc at the very least, maybe include it as part of lint-staged to ensure it's checked locally.

TylerAPfledderer · 2024-01-27T19:41:01Z

@pettinarip see the above comment. I'm going to stick this PR back into draft until I look at it further.

Basically, the errors in the build were actually what I was hoping for, but was not expecting it to stay in a preview deploy check. I'm going to take some time to play around with this. If it gets too complicated, I'll remove the strict typing flags from the theme script and look at those later.

I think restricting the typing if possible would help make sure typos or invalid token values are not overlooked and not bleed through.

TylerAPfledderer · 2024-01-27T23:51:50Z

After further triage, I have removed those strict typing flags from the scripts, put this PR back to "Ready for Review" and propose the following:

Open up GH issue for tracking to explore these flags separately. They are exposing typing issues that are hidden. Plus...
Set up a package script to run typescript checks locally, since this isn't done otherwise unless running the build script. This way, we are able to do a complete check of lints and type safety before pushing commits. (This could take care of itself via refactor: extend eslint #11934)

pettinarip · 2024-02-12T17:47:06Z

package.json

@@ -3,7 +3,8 @@
  "version": "8.0.0",
  "private": true,
  "scripts": {
-    "dev": "next dev",
+    "dev": "npx concurrently \"yarn next dev\" \"yarn theme:watch\"",


Agreed, this is more of a personal preference. In that sense, I'd prefer to keep the dev script as clean as possible.

However, I do see the benefit of running the Chakra script in parallel. We could add a comment in the readme or docs to let devs know how this works.

cc @wackerow @nhsz @corwintines

pettinarip · 2024-02-12T17:59:10Z

tsconfig.json

@@ -1,6 +1,6 @@
 {
  "compilerOptions": {
-    "target": "es5",
+    "target": "ESNext",


Won't this affect our browser support? if not sure, I'd recommend doing this in a separate PR to run tests accordingly.

TylerAPfledderer · 2024-04-05T03:22:31Z

tsconfig.json

@@ -1,6 +1,6 @@
 {
  "compilerOptions": {
-    "target": "es5",
+    "target": "ES6",


@pettinarip your question here and my response was removed due to a minor change I did.

So as to if this effects browser support, ES6 is supported by roughly 98% from caniuse.com.

The TypeScript docs of course also recommends ES6

Because there are dynamic keys in some component themes (i.e. [$border.variable]), the CLI will fail with the ES5 target because dynamic keys are part of theES6 spec.

wackerow · 2024-04-16T20:27:17Z

However, I do see the benefit of running the Chakra script in parallel. We could add a comment in the readme or docs to let devs know how this works.

@pettinarip Personally would just add a dev:watch script, but not a strong preference here.

pettinarip · 2024-04-24T12:11:47Z

However, I do see the benefit of running the Chakra script in parallel. We could add a comment in the readme or docs to let devs know how this works.

@pettinarip Personally would just add a dev:watch script, but not a strong preference here.

I'd prefer to minimize the number of scripts we have to make it simpler and more manageable. But if you think that adding that script will add value and will be used by a majority of devs, then I'd totally support that.

What I wouldn't want to happen is to have a lot of scripts that nobody uses.

pettinarip

LGTM! thanks @TylerAPfledderer

TylerAPfledderer added 2 commits January 27, 2024 11:09

build: add and set up @chakra-ui/cli

bf69647

build: use concurrently command for dev script

75c0dd0

TylerAPfledderer requested review from wackerow, corwintines, pettinarip, minimalsm and nhsz as code owners January 27, 2024 18:49

github-actions bot added dependencies 📦 Changes related to project dependencies internal 🏠 labels Jan 27, 2024

TylerAPfledderer changed the title ~~Build/chakra-cli~~ build: add Chakra CLI Jan 27, 2024

TylerAPfledderer commented Jan 27, 2024

View reviewed changes

TylerAPfledderer marked this pull request as draft January 27, 2024 19:44

revert(package.json): remove strict typing flags from theme gen scripts

e4b7c53

TylerAPfledderer marked this pull request as ready for review January 27, 2024 23:48

pettinarip requested changes Feb 12, 2024

View reviewed changes

TylerAPfledderer added 2 commits February 15, 2024 00:30

revert(package.json): simplify dev script back to next dev

29b5790

Merge remote-tracking branch 'origin/dev' into build/chakra-cli

4898946

github-actions bot added the internal 🏠 label Apr 5, 2024

chore(tsconfig): specify ES6 target

f5f9e57

TylerAPfledderer commented Apr 5, 2024

View reviewed changes

Merge remote-tracking branch 'origin/dev' into build/chakra-cli

b90df94

wackerow added needs dev approval 🧑‍💻 Approval from a developer is needed before merging and removed internal 🏠 labels Apr 16, 2024

Merge remote-tracking branch 'origin/dev' into build/chakra-cli

ba4f3d1

pettinarip approved these changes Apr 24, 2024

View reviewed changes

pettinarip merged commit 787bd87 into ethereum:dev Apr 24, 2024
6 checks passed

This was referenced Apr 24, 2024

Release candidate: v8.5.1 #12829

Merged

Deploy v8.5.1 #12830

Merged

TylerAPfledderer deleted the build/chakra-cli branch April 24, 2024 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: add Chakra CLI #12034

build: add Chakra CLI #12034

TylerAPfledderer commented Jan 27, 2024 •

edited

Loading

netlify bot commented Jan 27, 2024 •

edited

Loading

TylerAPfledderer left a comment

TylerAPfledderer Jan 27, 2024

pettinarip Feb 12, 2024

nhsz Feb 14, 2024 •

edited

Loading

TylerAPfledderer Feb 15, 2024

TylerAPfledderer Jan 27, 2024

TylerAPfledderer Jan 27, 2024 •

edited

Loading

TylerAPfledderer Jan 27, 2024 •

edited

Loading

pettinarip Feb 12, 2024

TylerAPfledderer Feb 13, 2024 •

edited

Loading

TylerAPfledderer Feb 17, 2024 •

edited

Loading

TylerAPfledderer commented Jan 27, 2024

TylerAPfledderer commented Jan 27, 2024

TylerAPfledderer commented Jan 27, 2024 •

edited

Loading

pettinarip Feb 12, 2024

pettinarip Feb 12, 2024

TylerAPfledderer Apr 5, 2024

wackerow commented Apr 16, 2024

pettinarip commented Apr 24, 2024

pettinarip left a comment

		"theme": "chakra-cli tokens ./src/@chakra-ui/theme.ts --strict-token-types --strict-component-types",
		"theme:watch": "chakra-cli tokens ./src/@chakra-ui/theme.ts --strict-token-types --strict-component-types --watch"

build: add Chakra CLI #12034

build: add Chakra CLI #12034

Conversation

TylerAPfledderer commented Jan 27, 2024 • edited Loading

Description

Related Issue

netlify bot commented Jan 27, 2024 • edited Loading

✅ Deploy Preview for ethereumorg ready!

TylerAPfledderer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhsz Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerAPfledderer Jan 27, 2024 • edited Loading

Choose a reason for hiding this comment

TylerAPfledderer Jan 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerAPfledderer Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

TylerAPfledderer Feb 17, 2024 • edited Loading

Choose a reason for hiding this comment

TylerAPfledderer commented Jan 27, 2024

TylerAPfledderer commented Jan 27, 2024

TylerAPfledderer commented Jan 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wackerow commented Apr 16, 2024

pettinarip commented Apr 24, 2024

pettinarip left a comment

Choose a reason for hiding this comment

TylerAPfledderer commented Jan 27, 2024 •

edited

Loading

netlify bot commented Jan 27, 2024 •

edited

Loading

nhsz Feb 14, 2024 •

edited

Loading

TylerAPfledderer Jan 27, 2024 •

edited

Loading

TylerAPfledderer Jan 27, 2024 •

edited

Loading

TylerAPfledderer Feb 13, 2024 •

edited

Loading

TylerAPfledderer Feb 17, 2024 •

edited

Loading

TylerAPfledderer commented Jan 27, 2024 •

edited

Loading