Skip to content

Add types for react-meteor-data package#377

Merged
Grubba27 merged 1 commit intometeor:masterfrom
alisnic:meteor-data-types
Jan 4, 2023
Merged

Add types for react-meteor-data package#377
Grubba27 merged 1 commit intometeor:masterfrom
alisnic:meteor-data-types

Conversation

@alisnic
Copy link
Copy Markdown
Contributor

@alisnic alisnic commented Nov 29, 2022

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Nov 29, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Copy Markdown
Contributor

@piotrpospiech piotrpospiech left a comment

Choose a reason for hiding this comment

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

I would prefer the types to be generated from the typescript files as done in react-meteor-accounts. It will be easier to maintain them.

@alisnic
Copy link
Copy Markdown
Contributor Author

alisnic commented Dec 1, 2022

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
Copy link
Copy Markdown
Contributor Author

alisnic commented Dec 2, 2022

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
Copy link
Copy Markdown
Contributor

@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
Copy link
Copy Markdown
Collaborator

@Grubba27 what is the status of this?

@Grubba27
Copy link
Copy Markdown
Contributor

Grubba27 commented Jan 4, 2023

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 Grubba27 merged commit 77b4912 into meteor:master Jan 4, 2023
@Grubba27
Copy link
Copy Markdown
Contributor

Grubba27 commented Jan 4, 2023

2.6.1 is out!

@alisnic
Copy link
Copy Markdown
Contributor Author

alisnic commented Jan 4, 2023

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
Copy link
Copy Markdown
Contributor Author

alisnic commented Jan 4, 2023

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
Copy link
Copy Markdown
Contributor

Grubba27 commented Jan 4, 2023

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
Copy link
Copy Markdown
Contributor Author

alisnic commented Jan 4, 2023

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
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor

ebroder commented Feb 8, 2023

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.

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.

6 participants