-
-
Notifications
You must be signed in to change notification settings - Fork 884
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
Leverage TypeScript project references #838
base: main
Are you sure you want to change the base?
Conversation
type Options, | ||
type UrlTransform, | ||
defaultUrlTransform, | ||
default |
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.
should be possible to do Markdown as default
, then you don‘t need the change in lib/index.js
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.
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.
@@ -138,7 +138,7 @@ const deprecations = [ | |||
* @returns {ReactElement} | |||
* React element. | |||
*/ | |||
export function Markdown(options) { |
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 change can be discarded, with the change in lib/exports.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.
No. lib/exports.ts
compiles to types/exports.d.ts
. That file describes the packages main entry point, which is lib/index.js
.
type UrlTransform, | ||
defaultUrlTransform, | ||
default | ||
} from './index.js' |
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 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.
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’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.
type UrlTransform, | ||
defaultUrlTransform, | ||
default | ||
} from './index.js' |
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 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.
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.
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.
"exports": { | ||
"types": "./types/exports.d.ts", | ||
"default": "./lib/index.js" | ||
}, |
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.
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'
?
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.
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", | ||
"index.js" | ||
"types/" |
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.
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.
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 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.
Co-authored-by: Titus <[email protected]> Signed-off-by: Remco Haszing <[email protected]>
Initial checklist
Description of changes
This changes the TypeScript configuration of the project. It’s based on https://github.com/orgs/unifiedjs/discussions/238. The project now uses 2 TypeScript configurations:
tsconfig.json
andtsconfig.build.json
. There’s alsotsconfig.base.json
. This is an implementation detail, it only exists so it can be extended from.tsconfig.build.json
is used to build the source files.lib
directory.types
directory.node
types, meaning that importing Node.js builtins will cause the build to fail.dom
lib, which is needed because of an upstream dependency.lib/exports.ts
, which can be used to re-export additional types. This replaces the oldindex.js
in the project root, which@typedef
tags to mimic type exports.tsconfig.json
is used to typecheck the rest of the project.lib
directory.node
types, meaning we can import Node.js builtins (such asnode:test
)dom
types, meaning we can’t use browser globals there.tsconfig.build.json
first, based onreferences
.types
directory emitted bytsconfig.build.json
. We have run into JSDoc specific emit issues before, so this is really nice to have.To build or rebuild the project, we can now run either
tsc --build
ortsc --build --force
. This correctly uses incremental builds, so a second run oftsc --build
is faster.Without this PR, after building, we have type errors in our editor. This is now solved.
Variations of this are possible. The main point is:
rootDir
andoutDir
..d.ts
files..ts
files to write TypeScript things that are not possible with types in JSDoc.An interesting idea that builds on this, is to move type definitions from
@typedef
tags into TypeScript files.