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 type errors #321

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix type errors #321

wants to merge 3 commits into from

Conversation

remcohaszing
Copy link
Member

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Since typical usage of unified-lint-rule involves inferring the type based on a function call, the inferred type should be public. If private types are used, TypeScript will generate types for dependant packages that use relative paths.

To solve this, public types in unified-lint-rule were moved from lib/index.js to index.js. lintRule only uses these public types.

The severity can be set as a boolean. To support this, the Label type now accepts a boolean type.

Two tests explicitly test bad input. These are annotated with a @ts-expect-error comment.

Since typical usage of `unified-lint-rule` involves inferring the type
based on a function call, the inferred type should be public. If private
types are used, TypeScript will generate types for dependant packages
that use relative paths.

To solve this, public types in `unified-lint-rule` were moved from
`lib/index.js` to `index.js`. `lintRule` only uses these public types.

The severity can be set as a boolean. To support this, the `Label` type
now accepts a boolean type.

Two tests explicitly test bad input. These are annotated with a
`@ts-expect-error` comment.
@remcohaszing remcohaszing added 🐛 type/bug This is a problem 👶 semver/patch This is a backwards-compatible fix ☂️ area/types This affects typings 🤞 phase/open Post is being triaged manually labels Apr 23, 2024
@wooorm
Copy link
Member

wooorm commented Apr 24, 2024

Since typical usage of unified-lint-rule involves inferring the type based on a function call, the inferred type should be public. If private types are used, TypeScript will generate types for dependant packages that use relative paths.

Why not make more types public? That would be a smaller PR, right?

The severity can be set as a boolean. To support this, the Label type now accepts a boolean type.

Can you do this in a separate PR? That way, we can focus on how to solve completely broken types here.


Also, you can use import('package-name') instead of import('../index.js'), with export maps.
I prefer that, as it shows that something is from the public API instead of from a private internal lib file.

This makes it clear the type is public. It also implicitly acts as a
test this type is public.
This is how it was previously exposed.
@remcohaszing
Copy link
Member Author

Why not make more types public? That would be a smaller PR, right?

Which type? SeverityTuple is only used internally.

Can you do this in a separate PR? That way, we can focus on how to solve completely broken types here.

That would cause a type error in the tests.

Also, you can use import('package-name') instead of import('../index.js'), with export maps. I prefer that, as it shows that something is from the public API instead of from a private internal lib file.

Nice, I like it.


Note that TypeScript 5.5’s @import annotation will allow us to add the type imports on top instead of using import('module').Type type annotations.

@ChristianMurphy
Copy link
Member

@remcohaszing @wooorm friendly ping 🔔

@remcohaszing
Copy link
Member Author

IMO this is good to go, although we might as well wait until TypeScript 5.5 now and use @import tags.

@wooorm
Copy link
Member

wooorm commented Jun 17, 2024

Which type? SeverityTuple is only used internally.

You describe this in your OP: “If private types are used, TypeScript will generate types for dependant packages that use relative paths.” The word private doesn’t seem right to me: if these types end up in the public API, they are thus publidc? If they are public, we should be expose to expose them too?


I feel weird about this PR because it does different things, one part of which is it uses a style that we don‘t use anywhere in unified (import(xx).yy expressions everywhere).

The other part of what this PR does (defining types in the root of a package) could perhaps be done and/or introduced everywhere, but I still wonder about my above point. And also if we’re going to do something along those lines, as I expression in the other issue, I am leaning more to using .d.ts files for type-only things.

@remcohaszing
Copy link
Member Author

The following code does not re-export PrivateOptions as PublicOptions:

// index.js
/**
 * @typedef {import('./lib/index.js').PrivateOptions} PublicOptions
 */

It defines a new type named PublicOptions that is equivalent to PrivateOptions. It’s kind of like this:

// index.js
import { privateFunction } from './lib/index.js'

export function publicFunction(...args) {
  return privateFunction(...args)
}

publicFunction and privateFunction are the equivalent, but they’re not the same function.

There are no type imports, type exports, nor type re-exports with types in JSDoc. Type imports are coming in TypeScript 5.5 though.


Calling const rule = lintRule() inferred a return type that uses Label, Option, and Severity that are defined in lib/index.js, a file that is not available through export maps. Hence those types are not part of the public API. The only way for TypeScript to access these types, is by using a relative path.


I want to provide a PoC for https://github.com/orgs/unifiedjs/discussions/238 once TypeScript 5.5 has been released, as that fixes some of the type issues we keep running into.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☂️ area/types This affects typings 🤞 phase/open Post is being triaged manually 👶 semver/patch This is a backwards-compatible fix 🐛 type/bug This is a problem
Development

Successfully merging this pull request may close these issues.

3 participants