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.

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: Publish types (+ validate and use types + generate type maps) #21

Merged
merged 13 commits into from
Mar 8, 2023
Merged

Fix: Publish types (+ validate and use types + generate type maps) #21

merged 13 commits into from
Mar 8, 2023

Conversation

voxpelli
Copy link
Contributor

@voxpelli voxpelli commented Oct 7, 2022

I noticed that this package was generating types but then told npm to only publish browser.js and index.js and not the actually generated types.

This fixes that + generates declarations maps + validates the types with TS strict=true.

It also adds a tsconfig.json to tell eg. VSCode that hey, this repository wants you to make use of TS types in index.js.

Tell me if you want me to pull something of this PR and I'll make it more granular 👍

Note: I bumped the lowest supported Node-version to a reasonable current LTS-focused version range so that the tsconfig.json file could be pointed towards a reasonable fitting definition with correct lib definition: es2020

Fixes #15

Closes #20

package.json Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
package.json Outdated
},
"files": [
"index.js",
"index.d.ts",
"index.d.ts.map",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it should publish a Source Map

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not? It's just a way to ensure that "go to source" in eg. VSCode actually still goes to the source and not to the type file?

And its only this big:

{"version":3,"file":"index.d.ts","sourceRoot":"","sources":["index.js"],"names":[],"mappings":"AA+BA,0CAHW;IAAE,KAAK,CAAC,EAAE,OAAO,GAAG,SAAS,CAAA;CAAE,GAC7B,OAAO,CAgFnB"}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed ✔️

tsconfig.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
voxpelli and others added 2 commits October 7, 2022 13:21
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
declaration.tsconfig.json Outdated Show resolved Hide resolved
Co-authored-by: LitoMore <LitoMore@users.noreply.github.com>
@voxpelli voxpelli requested review from LitoMore and sindresorhus and removed request for sindresorhus and LitoMore October 8, 2022 11:55
@voxpelli
Copy link
Contributor Author

voxpelli commented Mar 5, 2023

Anything I can do to help push this further?

package.json Outdated
"test": "xo && nyc ava",
"create-types": "tsc index.js --allowJs --declaration --emitDeclarationOnly"
"test": "xo && nyc ava && tsc",
"create-types": "tsc -p declaration.tsconfig.json"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use the long-flag rather than -p.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

index.js Outdated
function parseVersion(versionString) {
if (/^\d{3,4}$/.test(versionString)) {
// Env var doesn't always use dots. example: 4601 => 46.1.0
const m = /(\d{1,2})(\d{2})/.exec(versionString);
const m = /(\d{1,2})(\d{2})/.exec(versionString) ?? [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to either upgrade XO or not use ??. I suggest not using ?? as that's a less invasive change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sindresorhus sindresorhus merged commit a2546da into jamestalmage:master Mar 8, 2023
@sindresorhus
Copy link
Collaborator

@sindresorhus
Copy link
Collaborator

Not sure why master branch is failing when this PR passed.

https://github.com/jamestalmage/supports-hyperlinks/actions/runs/4363207180

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supporting typing
3 participants