-
-
Notifications
You must be signed in to change notification settings - Fork 125
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
refactor: use tsconfig monorepo #132
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit bf3e9da:
|
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.
I'm sorry, I had pending comments that I thought I submitted, but it's actually not.
a34f586
to
02190fa
Compare
Please make changes minimal as I'm working on #136 too. |
I will continue work on this today |
7a8d301
to
6b96508
Compare
f078b1d
to
5ee1aed
Compare
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 looks very nice.
I haven't used "references" before.
My preference is to have things as minimal as possible.
Some of tsconfig properties are new to me, and I can't judge, but please keep that in mind.
.github/workflows/ci.yml
Outdated
@@ -20,6 +20,21 @@ jobs: | |||
- run: pnpm install --frozen-lockfile | |||
- run: pnpm test | |||
|
|||
type-check: |
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.
We run tsc in pnpm test
, so it's not necessary here?
"outDir": "./dist", | ||
"rootDir": "./src" |
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.
Not sure why they are required, but maybe they are?
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.
it is required otherwise the output file structure will be
- dist
- src
- xxx.js
instead of
- dist
- xxx.js
It's not needed here since waku build
is real output files. But just some habit for me.
But it's necessary for packages/waku
since we're monorepo tsconfig now
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.
hm, that makes sense.
{ | ||
"path": "./tsconfig.node.json" | ||
} | ||
] |
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.
Why do we need this in 02_async, not in 01_counter?
I do not understand why two tsconfig files are required, instead of one.
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.
because vite.config.ts
is actually in a different environment with files in ./src
.
For example, you cannot include src files in config because of two ts configs with different files included
This is good for mono repo because sometimes you will mistakenly include files across the packages
Also it will be compiled to the different outputDir based on tsconfig.
01_counter doesn't have two configs because there is no vite.config.ts
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 for the explanation. We will revisit this later.
You can read the document here: https://www.typescriptlang.org/docs/handbook/project-references.html |
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.
Looking good. I feel like I need to learn some more later.
examples/02_async/tsconfig.json
Outdated
// React Server Async Component type | ||
"react/experimental" |
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 we put this also in the source code as inline comment? Just a preference, but it looks more explicit.
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 is remaining. Also in 10_dynamicroute.
packages/create-waku/tsconfig.json
Outdated
"module": "ESNext", | ||
"moduleResolution": "bundler", |
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.
I don't think this is correct for cli tools. I think it should be nodenext
, no?
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.
bundler is better? Since you are using esbuild here
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.
For now i think change it to nodenext wont affect anything
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.
Oh, I forgot that we use esbuild in cli. Yeah, let's use nodenext if we can. Using esbuild is only a temporary solution anyway.
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.
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.
In this case, create-waku is cjs module but we import a ESM module, I'm not sure the purpose of you're importing this
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.
importing fs-extra/esm ?
@vasucp1207 introduced it.
Maybe we should refactor it. My intention is everything should be ESM and no CJS in this repo. (and our plan was to avoid esbuild eventually.)
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.
I see, i can do that but it's too far away from this PR
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.
That would be great. Feel free to open a new PR.
5cde695
to
147c547
Compare
147c547
to
4193584
Compare
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.
we are almost there.
examples/02_async/tsconfig.json
Outdated
// React Server Async Component type | ||
"react/experimental" |
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 is remaining. Also in 10_dynamicroute.
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.
LGTM
Thanks for your contribution!
Add top-level ts config that includes all submodules, which restricts the type and isolated type context. If some files are missing or wrong-typed, it will throw the error.