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

Jump to bottom

Leverage TypeScript project references #838

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Leverage TypeScript project references #838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 2 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
coverage/
node_modules/
*.d.ts
types/
*.tsbuildinfo
*.log
Comment on lines +4 to 5
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
*.tsbuildinfo
*.log
*.log
*.tsbuildinfo

.DS_Store
react-markdown.min.js
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

} 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

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.

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

export default function Markdown(options) {
const allowedElements = options.allowedElements
const allowElement = options.allowElement
const children = options.children || ''
Expand Down
8 changes: 5 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +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'?

"files": [
"lib/",
"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.

],
"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"
}
}
14 changes: 14 additions & 0 deletions tsconfig.build.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
"extends": "./tsconfig.base.json",
"compilerOptions": {
"composite": true,
"declaration": true,
"emitDeclarationOnly": true,
"lib": ["dom", "es2022"],
"outDir": "types/",
"rootDir": "lib/",
"target": "es2022",
"types": []
},
"include": ["lib/"]
}
17 changes: 6 additions & 11 deletions tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,11 @@
{
"extends": "./tsconfig.base.json",
"compilerOptions": {
"checkJs": true,
"customConditions": ["development"],
"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