urql-graphql / urql

The highly customizable and versatile GraphQL client with which you add on features like normalized caching as you grow.
https://urql.dev/goto/docs
MIT License
8.65k stars 454 forks source link

fix(types): move `types` condition to the front #3204

Closed Andarist closed 1 year ago

Andarist commented 1 year ago

I moved types condition to the front. package.json#exports are order-sensitive - they are always matched from the top to the bottom. When a match is found then it should be used and no further matching should occur.

Right now, the current setup works in TypeScript but it's considered a bug and it should not be relied upon, see the thread and the comment here. For that reason, I would like to fix all popular packages that misconfigured their exports this way so the bug can be fixed in TypeScript.

⚠️ this PR focuses solely on fixing "🐛 Used fallback condition" problem but the "🎭 Masquerading as CJS" remains here. You can check the reported errors here

Andarist commented 1 year ago

With how exports were designed there is no room for a tool to use priorities. It's up to the package author to craft the priorities based on what conditions might be "enabled" by the user. Consider this example:

{
  "exports": {
    "import": { "types": "./index.d.mts" },
    "types": "./index.d.ts"
  }
}

When resolving using new Set(["import", "types"]) we should actually resolve with ./index.d.mts and not types": "./index.d.ts.

I agree that at first the design choices around exports might look odd but, as a user, I tried to imagine alternatives that would deliver the same flexibility and I couldn't. I think that the design makes sense - but at the same time, unfortunately, it's not always super intuitive. I think it's a format best authored and consumed by tools. We don't have many tools on the market that actually author exports based on simpler configuration though

kitten commented 1 year ago

I mean, I don't doubt it makes sense given that the usage that TS envisions is based on the constraints of what's available — after all, exports came before exports.types being a thing. Also, not your fault, I'm obviously just rambling 😅

Overall, I'd still put this squarely in the camp of exports having not enough strong design decisions in the first place — that said, I don't believe TS has helped the situation much.

One could argue of course that TS has little choices, but given it started from scratch, there for instance, could've been multiple identifiers for this, e.g. importTypes and requireTypes, which could then optionally point at the same file.

Andarist commented 1 year ago

I didn't take your post as targeted at me - nor I wanted to target anything at you. Just giving some additional context cause I spent in this hole way too much time than I should - and I enjoy the conversation 😅

could've been multiple identifiers for this, e.g. importTypes and requireTypes, which could then optionally point at the same file.

That's also not quite possible because we can have crazy things like:

{ "exports": { "development": { "react-native": { "ohLetsPutSomethingCustomHere": { "import": { "types": "./index.d.mts" } } } } } }

The point of exports is that you can create quite complex AND+OR branching using it (the end result isn't super readable but the flexibility to achieve crazy scenarios like this is there).