Remove duplicate copy of react-meteor-data.d.ts from isopack

[ebroder]

This is a followup to #377 based on my best understanding of how zodern:types expects type declarations to appear.

(FYI @zodern @piotrpospiech @radekmie in case any of y'all see something I've missed)

As discussed on #377, the current configuration does not seem to allow zodern:types to detect the type declarations for react-meteor-data. As I understand it, 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). This is because, in addition to explicitly including the definition file as an asset, the typescript package includes any .ts file (.d.ts or not).

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. While it deduplicates by the hash of definition files, the react-meteor-data package in Atmosphere does not seem to include the hash property for the asset version of the definition file, meaning it isn't deduped against the source versions. This means that zodern:types sees 2 different definition files in the package, and skips over it as being ambiguous.

We can fix this by not shipping the asset version of the definition file (since the non-asset version is already included). We also can drop the package-types.json file, which was never included in the resulting build and was thus vestigial.

I was able to test this locally by copying the react-meteor-data directory into a project, artificially bumping the version number (to 2.6.2_1), and running meteor update react-meteor-data to verify that it picked up the later version. After running meteor lint, I was able to successfully pick up types for react-meteor-data.

Here's what I saw before:

evan@mathias meteor-types-test (main~1) % meteor npx tsc
[...]
imports/ui/Info.tsx:2:28 - error TS2306: File '/Users/evan/src/meteor-types-test/.meteor/local/types/packages.d.ts' is not a module.

2 import { useTracker } from 'meteor/react-meteor-data';
                             ~~~~~~~~~~~~~~~~~~~~~~~~~~
[...]

After taking the update, that error message is gone.

[zodern]

This looks good.

As an alternative, I think removing api.addAssets('react-meteor-data.d.ts', 'server'); would also fix the issue, since then there would only be the one copy of the types definition added by the typescript package. That would also remove the need for the package-types.json file.

[ebroder]

I'm a little afraid to do that because I still don't understand exactly why the source version of the .d.ts file is getting included. Do you? It seems like this filter line should keep the file from getting passed to BabelCompiler, which should prevent it from being added at all, unless Isobuild just blindly adds all source files?

[zodern]

Meteor compiles the files in the package when it builds an app that uses the package, which is where the filter you linked to is run. When publishing, Meteor includes all files with the extensions and filenames passed to Plugin.registerCompiler at https://github.com/meteor/meteor/blob/01a25e77c89ba6e4fc298695819e6b2d65754349/packages/typescript/plugin.js#L2. zodern:types only requires the app to be published with the .d.ts file; it doesn't matter if the file is actually compiled.

[ebroder]

Ah, and that doesn't happen with package-types.json because there's no built-in compiler for JSON files? Hmm. I think I personally prefer the explicitness of this approach - having the file only be included because the typescript package is used feels a little too spooky action at a distance for me, although I guess that's sort of the name of the game with Meteor packages. Still, though.

[radekmie]

While I agree that the explicitness here is good, this is a package the community may look up for the integration example, and as such, it should be as simple as possible (and working, of course).

How about that: we can remove this line and release a new version. Then check if it works, and add both only if needed.

[ebroder]

Err...wait, will that approach work? I think that if we remove the package-types.json and stop shipping react-meteor-data.d.ts as an asset, then the end result will be:

• react-meteor-data.d.ts will be included as a typescript source file, but will still appear in each architecture's unibuild
• zodern:types will iterate over each unibuild and see multiple definition files
• And thus zodern:types will still skip over the package

[ebroder]

Aha. OK I remembered this morning that I figured out how to test this (copy the package into the packages/ directory of your project and it overrides the package on Atmosphere) and confirmed that this does work. What I missed here:

• zodern:types will iterate over each unibuild and see multiple definition files

is that zodern:types deduplicates by the hash of the definition file, so since the files in each isobuild have the same hash, everything is fine.

In fact, it seems like hash deduplication should be sufficient to have prevented this problem from ever occurring in the first place, but when I look at the isopack from Atmosphere, there's no hash for the asset version of the file:

  "resources": [
    {
      "type": "asset",
      "file": "os/packages/react-meteor-data/react-meteor-data.d.ts",
      "length": 1399,
      "offset": 0,
      "servePath": "/packages/react-meteor-data/react-meteor-data.d.ts",
      "path": "react-meteor-data.d.ts"
    },

In any case, I've rewritten this branch to drop the asset (and the now-vestigial package-types.json file).

[Grubba27]

Is it ready now for being released? I'm planning on making a new meteor release candidate with the types changes as well and it would be nice to test all together

[ebroder]

I believe matches what @radekmie requested and is good to go.

[radekmie]

I believe matches what @radekmie requested and is good to go.

That's exactly that! Sorry for not being clear enough in the first place.