Add a failing test case with types: null condition

[Andarist]

This is a test case related to this bug: #50762 .

It feels that this situation is specific-enough to be fixable regardless of what is decided for #50762 , cc @andrewbranch

[typescript-bot]

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

[Andarist]

i'm not sure how to avoid this in the test setup without allowJs: true (which I would prefer to avoid here).

[Andarist]

I feel like this should error because of the quite explicit types: null declaration in the package

[andrewbranch]

I agree that this is #50762 which makes it a bug, and I agree that it’s narrow enough to be fixable on its own, and at first glance at the conversation at discordjs/discord.js#9485 it even feels useful. But thinking about it a bit more, it feels a little suspicious to try to forbid usage of a package subpath by derailing TypeScript specifically. In my view, TypeScript’s job is to know how the runtime/bundler is going to resolve, and then to find types for the implementation file the runtime/bundler finds. Sometimes, though, you need to sacrifice a bit of the former in service of the latter—that is, if your types aren’t colocated with implementation files, you grab TypeScript with the "types" condition earlier in the process and say “look, use this file over here, and just trust me that it’s a representation of what the runtime/bundler will resolve to.” But the usage you suggest in the discord.js conversation would make the "types" condition a lie. I think it would be better for them to use other nested conditions, or move the subpath to "imports", or whatever, in order to make that subpath effectively private to consumers, which seems to be the effect they’re going for.

[Andarist]

Yeah, I overall agree with you - it's fishy to disallow something by disallowing types specifically. I don't know enough about their use case to suggest them a better way though. It just came up there since they thought that by not including types condition for this entrypoint they would forbid that at type-level.

However, because of the "used fallback condition" bug it didn't actually end up being forbidden there and even a more explicitly expressed intent (types: null) doesn't do that because it's still suffers from the same "used fallback condition" bug.

This particular PR is more about the fact that this should work and not as much about if it should be used 😉

[Andarist]

It's also very likely that this particular manifestation of this bug makes weird combinations to behave incorrectly, like import: null, require: './index.js'. Or perhaps even stuff like node: null, import: './index.mjs'.

[andrewbranch]

Do you want to see what the narrow fix for this would look like? If we don’t have to jump through hoops to fix null without fixing the rest of #50762, I’m not against it.

[Andarist]

Do you want to see what the narrow fix for this would look like?

Do I want to? Not really. Will I investigate this? Yeeeah, that's very likely.

[andrewbranch]

I’m going to steal this PR and fix it

[Andarist]

Be my guest, take It 😉

[andrewbranch]

Suppose you have

// @Filename: /a/b/c/node_modules/foo/package.json
{
  "exports": {
    ".": {
      "types": null
    }
  }
}

and

// @Filename: /a/b/c/node_modules/@types/foo/package.json
{
  "exports": {
    ".": {
      "types": "./index.d.ts"
    }
  }
}

Should the former block the latter? Should the former block a lookup in a higher-up node_modules directory?

[Andarist]

Yes. The first matched location like this should stop the algorithm from looking any further.

I’d probably extend this to just any condition (not just to types). If null value gets selected that is a pretty explicit sign that the matched entrypoint is forbidden/disallowed at runtime (and thus at types level too)

[andrewbranch]

I think this has some fairly unintuitive semantics. "types": null looks like it might mean “explicitly not typed,” but as you said, the fact that we arrived at null through "types" specifically is immaterial. If TS resolution arrives at a null, it should indicate that runtime resolution will also arrive at a null and stop further lookups.

If you want to tell TypeScript that something is explicitly not typed... you can point "types" at an untyped JavaScript file? 😅 But that won’t actually stop us from looking elsewhere for types.

I think this interpretation is perfectly consistent... it just struck me as weird. Implementation-wise, we actually have no existing mechanism to abort resolution with a failure. Sigh.

[jaridmargolin]

@andrewbranch - Are you still attempting to fix this? If I understand this thread correctly, I have what I would consider a similarly narrow issue. I am attempting to "hide" a subset of modules when using a custom "browser" condition. Would it make sense to create a new issue?

lib package.json:

  "exports": {
    ...
    "./entities/*/node": {
      "browser": null,
      "node": "./dist/entities/*/index.node.js"
    },
  }

consuming browser app tsconfig.json:

  "compilerOptions": {
    "customConditions": ["browser"]
  }

[andrewbranch]

Planning to fix this as a breaking change in 6.0 or 6.5, depending on whether it needs a deprecation message; I’m not sure we have the details settled.

[andrewbranch]

I'm pretty sure this will fail CI because the behavior changed for the better in #62483, which includes relevant tests. I just wanted to see before closing it permanently.