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.

Sign up for GitHub

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

fix(create-docusaurus): Fix TS issues on newly initialized sites #10694

Merged
merged 3 commits into from
Nov 19, 2024
Merged

fix(create-docusaurus): Fix TS issues on newly initialized sites #10694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b1021d1
e2e tests should run test site typechecking
slorber Nov 18, 2024
d0719f5
e2e tests should run test site typechecking
slorber Nov 18, 2024
a94b22b
Try to remove base tsconfig types
slorber Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions .github/workflows/tests-e2e.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,12 +160,15 @@ jobs:
- name: Installation
run: yarn || yarn || yarn
- name: Generate test-website project against main branch
run: yarn test:build:website -s
run: yarn test:build:website -st
- name: Install test-website project with npm
run: npm install
working-directory: ../test-website
env:
npm_config_registry: http://localhost:4873
- name: TypeCheck website
working-directory: ../test-website
run: yarn typecheck
- name: Start test-website project
run: npm run start -- --no-open
working-directory: ../test-website
Expand Down Expand Up @@ -197,15 +200,17 @@ jobs:
- name: Installation
run: yarn || yarn || yarn
- name: Generate test-website project against main branch
run: yarn test:build:website -s
run: yarn test:build:website -st
- name: Install test-website project with pnpm
run: |
npm install -g pnpm
# Fix some peer dependencies errors
pnpm add @algolia/client-search @types/react@17 typescript
pnpm install
working-directory: ../test-website
env:
npm_config_registry: http://localhost:4873
- name: TypeCheck website
working-directory: ../test-website
run: yarn typecheck
- name: Start test-website project
run: pnpm start --no-open
working-directory: ../test-website
Expand Down
5 changes: 0 additions & 5 deletions packages/docusaurus-tsconfig/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,6 @@
"moduleResolution": "bundler",
"module": "esnext",
"noEmit": true,
"types": [
"node",
"@docusaurus/module-type-aliases",
"@docusaurus/theme-classic"
],
Comment on lines -14 to -18
Copy link
Collaborator Author

@slorber slorber Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @Josh-Cena any opinion on this removal?

I thought types added packages for TS to look at, but it turns out it's the opposite and it restricts TS to only look at these types.

https://www.typescriptlang.org/tsconfig/#types

I misunderstood this config initially and not sure why we would use this instead of just having explicit dependencies. 🤔

More historical context about our TS preset is available here:

Wondering if this is safe to be considered as a bugfix for v3, or if I should change this later for v4.

The alternative is that our init template has to depend on @docusaurus/theme-classic + @types/node just because of pnpm (maybe yarn berry pnp too but tsc is disabled in CI atm):

CleanShot 2024-11-18 at 17 54 09@2x

Copy link
Collaborator Author

@slorber slorber Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Going to ship this in next patch and consider it a bugfix

Although it's not 100% retro-compatible, I think it's fine to consider that a site that use types without explicitly depending on them is a buggy site, and that it's ok to break TS typechecking for such sites and recommend site owners to add the missing dependencies.

If this was a bad decision, we can revert it.

"baseUrl": ".",
"paths": {
"@site/*": ["./*"]
Expand Down