-
Notifications
You must be signed in to change notification settings - Fork 214
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
Bumped react-script, seems to fix install and run issues #32
Conversation
I checked and it seems to me that this is not a complete solution, but a step in the right direction. When I used node v18.15.0 (npm v9.5.0) I was able to install all packages from But npm run start command fails with the error below
I also see that Typescript is not happy and modifies its configurations.
$ diff --git a/template/my_component/frontend/tsconfig.json b/template/my_component/frontend/tsconfig.json
index af10394..e18c413 100644
--- a/template/my_component/frontend/tsconfig.json
+++ b/template/my_component/frontend/tsconfig.json
@@ -1,7 +1,11 @@
{
"compilerOptions": {
"target": "es5",
- "lib": ["dom", "dom.iterable", "esnext"],
+ "lib": [
+ "dom",
+ "dom.iterable",
+ "esnext"
+ ],
"allowJs": true,
"skipLibCheck": true,
"esModuleInterop": true,
@@ -13,7 +17,10 @@
"resolveJsonModule": true,
"isolatedModules": true,
"noEmit": true,
- "jsx": "react"
+ "jsx": "react-jsx",
+ "noFallthroughCasesInSwitch": true
},
- "include": ["src"]
+ "include": [
+ "src"
+ ]
} This may not be related to the I also tried running our second template (
I am also not sure if this PR solves all the tickets you mentioned in the description of the PR. I too would like to let you know that the Warsaw team has it on the radar. I described these problems and proposed solutions in the document: 🐈⬛ Component development issues We wanted to tackle this at the beginning of the next quarter, and for now, focus on the goals of this quarter. CC: @sfc-gh-mnowotka |
I'm also wondering if we shouldn't also update the examples to use the newer version of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for me as well. Huzzah - thanks @blackary!
@sfc-gh-kbregula - thanks for looking about this more thoroughly. I'll let you guys handle the merging; feel free to close this PR if you'd rather wait to address all the issues! |
76d6be0
to
d4e50f3
Compare
96b718f
to
51661f5
Compare
51661f5
to
a7820f1
Compare
@tconkling I updated the PR. Now we have CI setup and in addition to templates, examples also work. Can I ask for review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for taking this on!
from pathlib import Path | ||
import json | ||
import sys | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a short docstring explaining the purpose of this script? Is it something we expect users to use, or is it just for Streamlit devs? (Should we include info about it in the repo's README?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for taking this on!
|
||
To list the available commands, run ./dev.py --help. | ||
""" | ||
import argparse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, if you're looking for other make alternatives, I really like https://taskfile.dev/
Fixes #29 #30 #21, I believe
Before:
After this change,
npm install
andnpm run start
run without errors