getObject() for Yup reports types of DefinitelyTyped

[karlhorky]

Hi @Haroenv and other maintainers 👋 Hope you are well! Thanks again for your continued effort on the projects in the Algolia ecosystem!

I'm not sure if I'm reporting this in the right place, but I wanted to reach out about what appears to be a data problem with npm-search index (we've only seen a single library so far):

Running index.getObject() with the package name yup returns the following:

{
  types: {
    ts: 'definitely-typed',
    definitelyTyped: '@types/yup',
  },
}

However, on the npm website, the yup package is reported as having built-in types and the @types/yup package has been deprecated:

Both the current version and the beta version of yup have no "types" or "typings" keys in the package.json:

• https://unpkg.com/browse/yup@0.32.11/package.json
• https://unpkg.com/browse/yup@1.0.0-beta.8/package.json

For the current yup@0.32.11, there is however no index.d.ts in the root:

• https://unpkg.com/browse/yup@0.32.11/index.d.ts (404)

But there is one in yup@1.0.0-beta.8:

• https://unpkg.com/browse/yup@1.0.0-beta.8/index.d.ts

Is the npm website or Algolia's index wrong? (seems like npm website is wrong 🤔)

---

Our usage:

const client = algoliasearch(
  'XXX',
  'xxxxxxxxxxx',
);

const index = client.initIndex('npm-search');

const results = await index.getObject<AlgoliaObj>(dependency, {
  attributesToRetrieve: ['types'],
});

const definitelyTypedPackageName = results.types?.definitelyTyped;

https://github.com/upleveled/preflight/blob/5af8a398ec370c79e22ab770307ec81d574c8e29/src/checks/noDependencyProblems/noDependenciesWithoutTypes.ts#L63-L67

[Haroenv]

I think the logic for deciding what's typed is wrong in the algolia index. npm correctly detects that there's .d.ts inside the es folder, but we don't.

I can see two possible improvements here:

1. make sure to exclude deprecated definitely-typed packages (does that count as builtin or no typescript support? usually it happens when the package adds their own TS definitions, but it could also be deprecated completely)
2. improve the check in this project which reads the file names so it also detects yup as having typescript support (if it's correct of course) (this file exists: https://unpkg.com/browse/yup@0.32.11/es/index.d.ts)

Are you interested in investigating those?

[karlhorky]

Oh, another problem, looks like Microsoft deleted the "create a search index" function in June:

microsoft/DefinitelyTyped-tools#456 (comment)

Maybe a new method is needed for determining this information:

npm-search/src/typescript/index.ts

Lines 24 to 40 in d99c31f

	export async function loadTypesIndex(): Promise<void> {
	const start = Date.now();
	const { body } = await request<TypeList[]>(config.typescriptTypesIndex, {
	decompress: true,
	responseType: 'json',
	});
	log.info(`📦 Typescript preload, found ${body.length} @types`);
	// m = modules associated
	// t = @types/<name>
	body.forEach((type) => {
	typesCache[unmangle(type.t)] = type.t;
	});
	datadog.timing('typescript.loadTypesIndex', Date.now() - start);
	}

[Haroenv]

Thanks for noticing that, that's indeed problematic. This probably means we need to do a reindex once this is fixed to catch packages that were added to DT since

[karlhorky]

Ok added a new issue for this, since it's a bit unrelated: #1090

[karlhorky]

I can see two possible improvements here:

1. make sure to exclude deprecated definitely-typed packages (does that count as builtin or no typescript support? usually it happens when the package adds their own TS definitions, but it could also be deprecated completely)
2. improve the check in this project which reads the file names so it also detects yup as having typescript support (if it's correct of course) (this file exists: unpkg.com/browse/yup@0.32.11/es/index.d.ts)

Are you interested in investigating those?

I think @Josehower would take over 2. above, I'll let him continue here.

[Josehower]

Hi @Haroenv thanks for your time,

I have been trying to setup the project on my machine so try to research a bit more about the problem or potential solutions to it, but I can't make the app to run locally is asking me a API key, i tried with the API key we use on one of our services but it didn't work.

➜  npm-search git:(master) yarn dev
Dec 6 15:22:24 🗿 npm ↔️ Algolia replication starts ⛷ 🐌 🛰 { version: '1.7.13' }
Dec 6 15:22:24 💪  Setting up Algolia OFCNCOG2CU [ 'npm-search-bootstrap', 'npm-search' ]
Dec 6 15:22:24 ⛑   API started on port 8000
Dec 6 15:22:25 {
  err: {
    name: 'ApiError',
    message: 'Method not allowed with this API key',
    status: 403,
    transporterStackTrace: [ [Object] ]
  }
}
  err: Error: Error during run
      at ./dist/src/index.js:99:23
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
Dec 6 15:22:25 Close was requested
Dec 6 15:22:25 Stopped Main gracefully

Wondering how can I get that API key to make it work of if there is an specific setup that need to be done, unfortunately I didn't have success with the setup on CONTRIBUTING.md

UPDATE: new error message after testing the setup on https://github.com/algolia/npm-search/blob/master/CONTRIBUTING.md

[Haroenv]

DM me on Twitter, I can set you up with a testing account for this :) it needs large limits

[jablko]

I think the TypeScript compiler will look for a bundled .d.ts file first, before an @types/package, so maybe this logic should be reversed:

npm-search/src/typescript/index.ts

Lines 69 to 87 in d99c31f

	// The 2nd most likely is definitely typed
	const defTyped = isDefinitelyTyped({ name: pkg.name });
	if (defTyped) {
	return {
	types: {
	ts: 'definitely-typed',
	definitelyTyped: `@types/${defTyped}`,
	},
	};
	}
	for (const file of filelist) {
	if (!file.name.endsWith('.d.ts')) {
	continue;
	}
	datadog.increment('jsdelivr.getTSSupport.hit');
	return { types: { ts: 'included' } };

I.e. if a package both contains built-in TypeScript declarations and a @types/package exists, the built-in declarations should take precedence.

[jablko]

That would solve the original problem in upleveled/preflight, I think: https://github.com/upleveled/preflight/blob/5af8a398ec370c79e22ab770307ec81d574c8e29/src/checks/noDependencyProblems/noDependenciesWithoutTypes.ts#L63-L67

where you're getting @types/yup for a package that contains built-in declarations.

[Haroenv]

The order was chosen for performance, as DT doesn't require any extra network requests. Maybe if it's deprecated we still fall through to "included" checks, but regularly we keep DT first?

[jablko]

I'm not familiar with how Algolia npm-search works, I'm kinda surprised yup falls through the first case:

npm-search/src/typescript/index.ts

Lines 64 to 67 in d99c31f

	// Already calculated in `formatPkg`
	if (pkg.types.ts === 'included') {
	return { types: pkg.types };
	}

Are GetPackage and NicePackage not the npm registry metadata, which contains "types": "./index.d.ts" for yup? https://registry.npmjs.org/yup

npm-search/src/formatPkg.ts

Lines 64 to 67 in d99c31f

	export function formatPkg(pkg: GetPackage): RawPkg | undefined {
	const start = Date.now();
	// Be careful NicePackage modify the Object ref
	const cleaned: NicePackageType | undefined = new NicePackage(pkg);

npm-search/src/formatPkg.ts

Line 134 in d99c31f

const types = getTypes(cleaned);

[karlhorky]

Maybe it's taking the current version yup@0.32.11 which does not have "types": "./index.d.ts":

https://registry.npmjs.org/yup/0.32.11

[karlhorky]

Wonder if that's actually a bug / missing feature in the npm feature of adding types information to the registry:

• RFC: Adding types information to the Package JSON in the registry npm/rfcs#126

(because in this case, the index.d.ts file for yup@0.32.11 is in the esm/ directory)

cc @orta

[jablko]

Probably that version of yup was published with a version of the npm CLI/pacote that hadn't implemented the RFC yet?

[jablko]

The order was chosen for performance, as DT doesn't require any extra network requests. Maybe if it's deprecated we still fall through to "included" checks, but regularly we keep DT first?

If Algolia npm-search indexes all the npm packages, and there are only about 8,000 DT types, is the performance of keeping DT first significant?

[Josehower]

I already tested the solution from @jablko and it seems to work fine

#1089 (comment)

if @Haroenv approves I can create the PR in no time

[Josehower]

I have created a PR that applies fix from @jablko

#1091

good energy 👍👍👍👍

[karlhorky]

@Josehower @Haroenv Thanks for creating and merging #1091 🙌

I think this hasn't taken into account the second improvement that @Haroenv mentioned above though:

1. make sure to exclude deprecated definitely-typed packages (does that count as builtin or no typescript support? usually it happens when the package adds their own TS definitions, but it could also be deprecated completely)

@Haroenv can you reopen this? Or should I create a new issue for this improvement?

[Haroenv]

I think with the changed order, the point 1 no longer is needed, unless you see some cases where that would still be relevant?