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

deno check error resolving package config for npm react jsx-runtime #18203

Closed
lrowe opened this issue Mar 15, 2023 · 16 comments · Fixed by #23419
Closed

deno check error resolving package config for npm react jsx-runtime #18203

lrowe opened this issue Mar 15, 2023 · 16 comments · Fixed by #23419
Labels
bug Something isn't working node compat

Comments

@lrowe
Copy link
Contributor

lrowe commented Mar 15, 2023

While importing the npm:@types packages as per #16653 silences errors from the language server when running deno check I see the following error:

% deno check index.tsx
Check file:///Users/lrowe/scratch/react-server-deno/index.tsx
error: Uncaught Error: Error resolving package config for 'npm:react@18.2.0/jsx-runtime': [ERR_PACKAGE_PATH_NOT_EXPORTED] Package subpath './jsx-runtime' is not defined by "exports" in '/Users/lrowe/Library/Caches/deno/npm/registry.npmjs.org/react/18.2.0/package.json' imported from '/Users/lrowe/Library/Caches/deno/npm/registry.npmjs.org/react/18.2.0/'
    at <anonymous> (ext:deno_tsc/99_main_compiler.js:592:28)

However in react's package.json an entry for './jsx-runtime' exists:

  "exports": {
    ".": {
      "react-server": "./react.shared-subset.js",
      "default": "./index.js"
    },
    "./package.json": "./package.json",
    "./jsx-runtime": "./jsx-runtime.js",
    "./jsx-dev-runtime": "./jsx-dev-runtime.js"
  },

Files to reproduce:

index.tsx:

// See: https://github.com/denoland/deno/issues/16653
import type {} from "npm:@types/react@18";
import type {} from "npm:@types/react-dom@18";

import React from "npm:react@18";
import { renderToString } from "npm:react-dom@18/server";

function Hello() {
  return <div>Hello world!</div>;
}

console.log(renderToString(<Hello />));

deno.json:

{
  "compilerOptions": {
    "jsx": "react-jsx",
    "jsxImportSource": "npm:react@18"
  }
}
% deno --version      
deno 1.31.2 (release, aarch64-apple-darwin)
v8 11.0.226.19
typescript 4.9.4
@dsherret dsherret added bug Something isn't working node compat labels Mar 15, 2023
@KyleJune
Copy link
Contributor

I've encountered this same issue. It is preventing me from switching from using esm.sh to using npm specifiers. Are there any plans to fix this issue anytime soon?

error: Uncaught Error: Failed resolving package config for 'npm:react@18.2.0/jsx-runtime': [ERR_PACKAGE_PATH_NOT_EXPORTED] Package subpath './jsx-runtime' is not defined for types by "exports" in '/home/kyle/.cache/deno/npm/registry.npmjs.org/react/18.2.0/package.json' imported from '/home/kyle/.cache/deno/npm/registry.npmjs.org/react/18.2.0/'
    at <anonymous> (ext:deno_tsc/99_main_compiler.js:676:28)

@nayeemrmn
Copy link
Collaborator

This is just #20455, which is on the roadmap but not currently being worked on.

The following workaround doesn't work:

// @deno-types="npm:@types/react@18"
import React from "npm:react@18";
// @deno-types="npm:@types/react-dom@18/server"
import { renderToString } from "npm:react-dom@18/server";

function Hello() {
  return <div>Hello world!</div>;
}

console.log(renderToString(<Hello />));

because it doesn't apply to the "jsxImportSource": "npm:react@18" in your compiler options. There's no way of tagging that one.

@dsherret
Copy link
Member

dsherret commented Sep 12, 2023

What about adding a "jsxImportSourceTypes" : "npm:@types/react@18" option? I think that would be better than #20455 for the reasons I outlined in that issue and you'd be able to specify types when there's a version mismatch between the @types package and the actual package. Another benefit is it's right beside where you define jsxImportSource so that makes it easier to update both at the same time.

@KyleJune
Copy link
Contributor

KyleJune commented Sep 12, 2023

That would be better but I'm guessing people will regularly miss that they need to set that types option too. Then they'd only find out if they google the error and find issues like this one.

Then we still have the problem that you'll need to add a bunch of these comments in any files that import react or other dependencies that have separate types.

// @deno-types="npm:@types/<package>"

I feel like it would be better to have a version mismatch in some cases than to have no types at all. I'd rather only have to add those comments in the case there is a mismatch issue than to need them for most modules.

The way esm.sh picks the types to use seems to just work the way you would expect it to. I don't have to manually set or override types for any of my esm.sh dependencies.

@nayeemrmn
Copy link
Collaborator

A nice general solution would be to extend our compilerOptions::types field (which is already incompatible with tsc) to support type associations:

{
  "compilerOptions": {
    "types": [
      "./types.d.ts",
      "https://deno.land/x/pkg@1.0.0/types.d.ts",
      { "target": "https://deno.land/x/some_untyped_module/", "types": "npm:@types/some-untyped-module/" },
      { "target": "npm:react@18", "types": "npm:@types/react@18" },
      { "target": "npm:react@18/", "types": "npm:@types/react@18/" }
    ]
  }
}

This would be yet another, but meaningfully distinct way of adding types to untyped modules. In order of priority:

  1. // @deno-types at the import site.
  2. { "target": "npm:react@18/", "types": "npm:@types/react@18/" } in project config. (new)
  3. /// <reference types="..." /> in the module itself.
  4. X-Typescript-Types: ... header that the module is served with.

It would satisfy the need to out-scope "jsxImportSource", you won't need to write duplicate @deno-types directives and this seems like a sensible configuration item to have independently of this issue.

It's comparable to adding the @types/react entry to your package.json.

@dsherret
Copy link
Member

dsherret commented Sep 12, 2023

@nayeemrmn seems complex. That would require keeping the react version in sync in 5 places instead of once for react and once for @types/react.

@KyleJune
Copy link
Contributor

KyleJune commented Sep 12, 2023

I like the idea of it just being something you add to deno.json. although if we add it to the types array, it would be nicer if we could just add the string "npm:@types/react@18.0.2" without having to specify a target or have 2 entries like you do where one has a trailing slash. In package.json you would just add the types to your list of dependencies without having to specify they are the types for react.

Ideally we would either not have to specify types or just have one place to specify them for the whole project.

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Sep 12, 2023

t would be nicer if we could just add the string "npm:@types/react@18.0.2"

That's a good idea, since npm: specifiers are graced it makes sense to allow that. If npm:@types/foo is detected in that types field, it's used for untyped npm:foo/<path> specifiers:

{
  "compilerOptions": {
    "types": [
      "./types.d.ts",
      "https://deno.land/x/pkg@1.0.0/types.d.ts",
      // Will be used for any `npm:react@18.x.x/*`
      "npm:@types/react@18"
    ]
  }
}

@dsherret WDYT?

@KyleJune
Copy link
Contributor

KyleJune commented Sep 12, 2023

The only additional thing I would want is for it to be able to work with an import map, that way I could just specify the version there instead of having to update both deno.json and my import map to bump my react version.

I think adding a types array entry like that would be the easiest way to do it as it would get rid of needing to add a bunch of deno-types comments all over your codebase.

@dsherret
Copy link
Member

dsherret commented Sep 12, 2023

@nayeemrmn I like that too 👍

@KyleJune you can store the import map in a deno.json nowadays (just copy and paste the "imports" and "scopes" properties into the deno.json, delete the "importMap" property in deno.json, then delete the import map file). I don't think we could support this in an import map file because it would be non-standard.

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Sep 12, 2023

I assume compilerOptions::types entries are put through the import map, so I think you'd naturally be able to do:

{
  "compilerOptions": {
    "types": [
      "./types.d.ts",
      "https://deno.land/x/pkg@1.0.0/types.d.ts",
      // If this resolves to `npm:@types/react@18`, it will be used for any `npm:react@18.x.x/*`
      "@types/react"
    ]
  },
  "imports": {
    "@types/react": "npm:@types/react@18"
  }
}

Though I wouldn't personally bother if they're in the same file anyway.

@KyleJune
Copy link
Contributor

KyleJune commented Sep 12, 2023

As long as my bare specifier alias for npm specifier imports can get the types I'm good then. I just don't want to have to update all my files to use npm specifier imports with the version in it since that would make updates difficult.

If types will still work for my bare specifiers and I put my import map in my deno.json, then I've got it where I could update my dependencies by only touching a single file.

@oscarotero
Copy link

I like the idea of including the types property inside compilerOptions, but I'd rather to be more explicit about which module is affected by the types.

To me, assuming that npm:@types/react@18 affects to any npm:react@18.x.x import is too magical and probably it would work with @types/* but not neccesarily to other packages (or non-npm packages).

I think a format more similar to import maps is more clear.

{
  "compilerOptions": {
    "jsx": "react-jsx",
    "jsxImportSource": "npm:react",

    "types": {
      "npm:react": "npm:@types/react@18",
      "https://deno.land/x/other-library/": "./my-custom-types.d.ts",
    }
  }
}

@farsightsoftware
Copy link

FWIW, a view from the cheap seats from this relative newcomer:

It seems a bit odd to put this in compilerOptions, since it's not a TypeScript compiler option, it's a Deno option. (And there's always the quite-small chance TypeScript would want to add a types option someday.)

Perhaps an import-types section in deno.json, keyed (as suggested above by @oscarotero) by the module specifier?

{
    "imports": {
        "react": "npm:react@^18.2"
    },
    "import-types": {
        "react": "npm:@types/react@^18.2"
    }
}

The entry in import-types would do two things:

  1. Import the types from the given source.
  2. Associate them with the main module like @deno-types=... does in all places the mapped specifier ("react") or its resolved full specifier ("npm:react@^18.2") is used in an import (or similar, including an implicit one for JSX).

A couple of notes re that example:

  • I've used the mapped specifier ("react") as the key in import-types. I don't know if it would be worth allowing the full specifier as well ("npm:react@^18.2": "npm:@types/react@^18.2"). It adds a third place to maintain the version, which seems silly. The only use case I can immediately think of is if you aren't using a mapped specifier (you're actually writing import ... from "npm:react@^18.2"; everywhere) and...why would you do that? 😊
  • I've used imports in the above, but conceptually there's no reason this couldn't continue to support importMap with a separate map. import-types would remain in deno.json to avoid making the separate map non-standard.

Separately, having to repeat the version number is a maintenance chore (one that Node's package.json also has), since in the normal case you want the same version as the package. Deno already improves on Node.js and npm in many ways, perhaps another would be to allow opting-in to inferring the version number from the main package? For instance, using @infer:

{
    "imports": {
        "react": "npm:react@^18.2"
    },
    "import-types": {
        "react": "npm:@types/react@infer"
    }
}

There, Deno would infer ^18.2 on npm:@types/react from the react mapping in imports. But if you gave one explicitly ("react": "npm:@types/react@18.1", perhaps because the types are lagging the main package a bit), it would use the version given.

To me, that would be an excellent devex for these packages that don't directly provide types.

(Using @infer if there's no version to infer from would be an error.)

Apologies if any of this is unworkable in ways that would be obvious to someone more experienced with the project.

@farsightsoftware
Copy link

One thing I missed in my comment above is that there already is a types section in TypeScript's compilerOptions (it's even mentioned in the Deno documentation). It's defined as an array of package names whose types should be in global scope (overriding the default behavior of including all node_modules/@types/___ packages in any enclosing folder).

I think my import-types suggestion remains valid, though, and plays well with types (for ambient type stuff a'la JSX). It's just embarrassing to have missed the existing types section (I could swear I'd checked. I'm not sure which is worse, not having checked or having missed it if I did...😐).

@dsherret
Copy link
Member

dsherret commented May 1, 2024

This is now possible to specify via jsxImportSourceTypes in Deno 1.43 (pragma or compiler option):

{
  "compilerOptions": {
    "jsx": "react-jsx",
    "jsxImportSource": "npm:react@^18.3",
    "jsxImportSourceTypes": "npm:@types/react@^18.3"
  }
}

https://deno.com/blog/v1.43#jsximportsourcetypes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working node compat
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants