meteor / react-packages

Meteor packages for a great React developer experience
http://guide.meteor.com/react.html
Other
573 stars 157 forks source link

Add types for react-meteor-data package #377

Closed alisnic closed 1 year ago

alisnic commented 1 year ago

Types are taken from https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/meteor/react-meteor-data.d.ts and updated to match the latest release.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

alisnic commented 1 year ago

Silly me 😁 I kind of assumed that if types are in types/meteor package the code is not written in TypeScript. Will update the PR

alisnic commented 1 year ago

The problem I have now is that typescript generates either separate type files or one with different modules:

$ tsc index.js -d --emitDeclarationOnly --out types.d.ts --allowJs --jsx react --esModuleInterop

$ cat types.d.ts
declare module "useTracker" {
    import { Tracker } from 'meteor/tracker';
    import { DependencyList } from 'react';
    export interface IReactiveFn<T> {
        (c?: Tracker.Computation): T;
    }
    export interface ISkipUpdate<T> {
        <T>(prev: T, next: T): boolean;
    }
    function useTrackerClient<T = any>(reactiveFn: IReactiveFn<T>, skipUpdate?: ISkipUpdate<T>): T;
    function useTrackerClient<T = any>(reactiveFn: IReactiveFn<T>, deps?: DependencyList, skipUpdate?: ISkipUpdate<T>): T;
    export const useTracker: typeof useTrackerClient;
}
declare module "withTracker" {
    import React from 'react';
    type ReactiveFn = (props: object) => any;
    type ReactiveOptions = {
        getMeteorData: ReactiveFn;
        pure?: boolean;
        skipUpdate?: (prev: any, next: any) => boolean;
    };
    export const withTracker: (options: ReactiveFn | ReactiveOptions) => (Component: React.ComponentType) => React.ForwardRefExoticComponent<React.RefAttributes<unknown>> | React.MemoExoticComponent<React.ForwardRefExoticComponent<React.RefAttributes<unknown>>>;
}

Any ideas how can I work around that?

piotrpospiech commented 1 year ago

@alisnic You can generate separate type files (just use --outDir types). I think it will work if you will set the typesEntry in the package-types to the index.d.ts after that.

You can also use --declarationMap to generate .d.ts.map files.

StorytellerCZ commented 1 year ago

@Grubba27 what is the status of this?

Grubba27 commented 1 year ago

Hey @StorytellerCZ, I was waiting for @piotrpospiech to approve these changes, but I've checked and reviewed it. I think it is good. I will make a new Release for react-meteor-data as soon as I can.

Grubba27 commented 1 year ago

2.6.1 is out!

alisnic commented 1 year ago

So after testing this does not seem to work 🤔 . Steps I did:

  1. Bump react-meteor-data to 2.6.1 in .meteor/packages
  2. ran meteor lint to re-generate types
  3. checked /path/to/meteor/project/.meteor/local/types/packages.d.ts and react-meteor-data type entry is missing

What did I miss in the PR?

alisnic commented 1 year ago

Apparently, I needed to add zodern:types as a dependency to the package. See https://github.com/zodern/meteor-types#meteor-packages. Should I open a PR?

Grubba27 commented 1 year ago

~In the project that you have created it does have zodern:types?~ Edit: Read it wrong, maybe added it to dependency. Sure! add it as dependency and I will make a new release

alisnic commented 1 year ago

I'll investigate a bit more, as the entry I referenced seem to be related to the process of automatically generating types on publish, which is not the use-case here. Types are already generated, we only need to expose them

piotrpospiech commented 1 year ago

Hi @alisnic, the zodern:types package and type definitions are implemented correctly. We don't need zodern:types in this package, because, as you said, it is needed only to generate types.

I was curious that it didn't work, so I tested it. I created a new Meteor project with a Typescript template. It installed v2.6.0 of react-meteor-data. First, I installed zodern:types. After that, I updated react-meteor-data to v2.6.1 and started the project, but no types were added (only types from other packages).

I removed react-meteor-data from the project and installed it again. After running meteor lint, types were added correctly.

It seems like it is an issue with the zodern:types package. Packages that were updated with type definitions are not detected. FYI @zodern

ebroder commented 1 year ago

OK. I did some digging into what's going on here. I wasn't able to reproduce @piotrpospiech's experience - my experience is that types for react-meteor-data are never discoverable.

I think the easiest workaround for this is to include package-types.json as an additional server-side asset, on top of react-meteor-data.d.ts. If zodern:types detects that, it'll prefer it over a lone .d.ts file being included in the repo.

The reason the current configuration doesn't work is because react-meteor-data.d.ts is being included as both an asset and as a source file. You can see this by looking at ~/.meteor/packages/react-meteor-data/2.6.2/os.json (and similarly the files for web.browser.json and web.browser.legacy.json). I'm not entirely sure why that's happening, although I suspect the typescript package is partially to blame here. Even though it seems like it should exclude .d.ts files from the resulting isopack, it does any .ts file as a source file (including .d.ts files).

In any case, when consuming types from Meteor packages, zodern:types looks for .d.ts files and uses them to provide types for the package, but only if there's a single .d.ts file. Because react-meteor-data ends up shipping 2 (actually 4, since the source files show up in all unibuilds, not just the server one), zodern:types skips over the package without pulling any definitions in.

I'll push up a PR in a sec to add the package-types.json file, although I frankly don't understand Isobuild well enough to figure out how to test any of this pre-merge.