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

Leverage TypeScript project references #838

Closed
wants to merge 4 commits into from
Closed

Leverage TypeScript project references #838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c17b71d
Leverage TypeScript project references
remcohaszing Jun 28, 2024
f8004c0
Update .gitignore
remcohaszing Dec 20, 2024
d7c6923
Re-enable declaration maps
remcohaszing Dec 20, 2024
45cac21
Merge branch 'main' into reconfigure-typescript
remcohaszing Dec 20, 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
3 changes: 1 addition & 2 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
coverage/
node_modules/
*.d.ts.map
*.d.ts
types/
*.log
*.tsbuildinfo
.DS_Store
Expand Down
9 changes: 0 additions & 9 deletions index.js
View file
Open in desktop

This file was deleted.

9 changes: 9 additions & 0 deletions lib/exports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export type {ExtraProps} from 'hast-util-to-jsx-runtime'
export {
type AllowElement,
type Components,
type Options,
type UrlTransform,
defaultUrlTransform,
default
Copy link
Member

Choose a reason for hiding this comment

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

should be possible to do Markdown as default, then you don‘t need the change in lib/index.js here

Copy link
Member Author

Choose a reason for hiding this comment

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

No. lib/index.js exports the actual runtime package values. exports.ts describes the package values. These exports must be in sync. Since lib/index.js has a default export, so should exports.ts.

I would love to change the export of this package to a named one, but that’s out of scope and a breaking change.

} from './index.js'
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be an lib/export.d.ts, with a different lib/export.js focussing on the values.
The primary goal is to not use a proprietary closed-governance compile-to-javascript language. Having types is fine. But not at the cost of having to compile.
Secondary, compile-to-javascript(/types) languages in my experience are bad at compiling. We can get better code by writing them manually.
TS has been generating bad or slow d.ts types. I think we should write those ourselves.

Copy link
Member Author

Choose a reason for hiding this comment

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

We’re still not compiling anything to JavaScript with these changes.

TypeScript is actually really good at generating d.ts files. TypeScript is written by humans, so sure it’s imperfect, and we have found some actual bugs. However, we’re using TypeScript wrong at a fundamental level. This also causes issues. This is what I’m trying to change.

We could indeed write d.ts files ourselves instead of compiling anything at all. It’s how we started. I think that would be a big step backwards though. It means we need to keep things in sync, and also we’d lose Go to Definition support. If we do decide to go that way, it needs to be a well informed choice, not based on an incorrect TypeScript setup.

Copy link
Member

Choose a reason for hiding this comment

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

I think it’s sad that we can no longer have index.js, and have to resort to lib/exports.*.
An index.js in the root is useful when getting into a new project: you know it’s going to include the API. Now we don’t have that.

Copy link
Member Author

Choose a reason for hiding this comment

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

The entry point of a package is package.json, not index.js. Node.js defaults the main entry point to index for CJS packages, but this isn’t true for ESM.

While a file named index.js in the package root is common, it’s far from the only common convention. This PR changes it to lib/index.js, which is also a common convention.

2 changes: 1 addition & 1 deletion lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ const deprecations = [
* @returns {ReactElement}
* React element.
*/
export function Markdown(options) {
Copy link
Member

Choose a reason for hiding this comment

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

this change can be discarded, with the change in lib/exports.ts

Copy link
Member Author

Choose a reason for hiding this comment

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

No. lib/exports.ts compiles to types/exports.d.ts. That file describes the packages main entry point, which is lib/index.js.

export default function Markdown(options) {
const allowedElements = options.allowedElements
const allowElement = options.allowElement
const children = options.children || ''
Expand Down
9 changes: 5 additions & 4 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,13 @@
],
"sideEffects": false,
"type": "module",
"exports": "./index.js",
"exports": {
"types": "./types/exports.d.ts",
"default": "./lib/index.js"
},
Copy link
Member

Choose a reason for hiding this comment

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

Is it correct that the types field here is only needed because you want to overwrite lib/index.d.ts, for consumers of 'react-markdown'?

Copy link
Member Author

Choose a reason for hiding this comment

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

The types field here is needed, because the runtime code and the type definitions are not next to each other.

In a TypeScript project, the source code is written in a .ts file. Then a runtime file .js and a .d.ts file are generated next to each other. In that case the types export isn’t needed.

When using types in JSDoc, the source file is the runtime file. Because you’re supposed to separate the generated output from the source directory, we need to use package exports to point the types condition to somewhere other than the default condition.

"files": [
"lib/",
"index.d.ts.map",
"index.d.ts",
"index.js"
"types/"
Copy link
Member

Choose a reason for hiding this comment

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

What does the generated folder structure look like?
I am not happy about the extra folder. Feels like this is patching a bug that TS should solve.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the folder structure after running tsc -b (omitting node_modules).

├── changelog.md
├── lib
│   ├── exports.ts
│   └── index.js
├── license
├── package.json
├── readme.md
├── script
│   └── load-jsx.js
├── test.jsx
├── tsconfig.base.json
├── tsconfig.build.json
├── tsconfig.build.tsbuildinfo
├── tsconfig.json
└── types
    ├── exports.d.ts
    └── index.d.ts

As you can see there are 2 generated files/folders:

  • tsconfig.build.tsbuildinfo
  • types/

There’s now a clear distinction between generated files and source files. There’s no more confusion for neither TypeScript, editor tooling, or authors. The lib directory is always clean. We don’t have to specify weird glob patterns or file exceptions in tsconfig.json include/exclude patterns. No type declarations are emitted for any files other than the source files. And as a bonus, the tests are type-checked against the emitted d.ts files, meaning those are tested too.

],
"dependencies": {
"@types/hast": "^3.0.0",
Expand Down
10 changes: 10 additions & 0 deletions tsconfig.base.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"compilerOptions": {
"checkJs": true,
"customConditions": ["development"],
"exactOptionalPropertyTypes": true,
"module": "node16",
"strict": true,
"target": "es2022"
}
}
15 changes: 15 additions & 0 deletions tsconfig.build.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"extends": "./tsconfig.base.json",
"compilerOptions": {
"composite": true,
"declaration": true,
"declarationMap": true,
"emitDeclarationOnly": true,
"lib": ["dom", "es2022"],
"outDir": "types/",
"rootDir": "lib/",
"target": "es2022",
"types": []
},
"include": ["lib/"]
}
18 changes: 6 additions & 12 deletions tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,11 @@
{
"extends": "./tsconfig.base.json",
"compilerOptions": {
"checkJs": true,
"customConditions": ["development"],
"declarationMap": true,
"declaration": true,
"emitDeclarationOnly": true,
"exactOptionalPropertyTypes": true,
"jsx": "preserve",
"lib": ["dom", "es2022"],
"module": "node16",
"strict": true,
"target": "es2022"
"lib": ["es2022"],
"noEmit": true,
"types": ["node"]
},
"exclude": ["coverage/", "node_modules/"],
"include": ["**/*.js", "**/*.jsx", "lib/complex-types.d.ts"]
"exclude": ["coverage/", "lib/", "node_modules/"],
"references": [{"path": "./tsconfig.build.json"}]
}
Loading