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

ESM X.test which have zero export statement, should be sub type of ESM X which have at least one export statement #57735

Closed
loynoir opened this issue Mar 12, 2024 · 11 comments
Labels
Not a Defect This behavior is one of several equally-correct options

Comments

@loynoir
Copy link

loynoir commented Mar 12, 2024

🔎 Search Terms

  • dynamic import

  • type union

  • no export

  • export {}

  • export default {}

🕗 Version & Regression Information

⏯ Playground Link

No response

💻 Code

reproduce.ts

async function optionalImport(id: string) {
  switch (id) {
    case 'a':
      return await import('./a.js')
    case 'a.test':
      return await import('./a.test.js')
  }

  throw new Error()
}

export { optionalImport }

a.ts

export const foo = 42

🙁 Actual behavior

  • When a.test.ts
import { foo } from './a.js'
describe('reproduce', () => { it('reproduce', () => { foo }) })

type is

function optionalImport(id: string): Promise<typeof import("/path/to/reproduce/a.test")>
  • When a.test.ts
import { foo } from './a.js'
describe('reproduce', () => { it('reproduce', () => { foo }) })
export {}

type is

function optionalImport(id: string): Promise<typeof import("/path/to/reproduce/a.test")>
  • When a.test.ts
import { foo } from './a.js'
describe('reproduce', () => { it('reproduce', () => { foo }) })
export default {}

type is

function optionalImport(id: string): Promise<typeof import("/path/to/reproduce/a") | typeof import("/path/to/reproduce/a.test")>

🙂 Expected behavior

When a.test.ts

import { foo } from './a.js'
describe('reproduce', () => { it('reproduce', () => { foo }) })

type is

function optionalImport(id: string): Promise<typeof import("/path/to/reproduce/a") | typeof import("/path/to/reproduce/a.test")>

Additional information about the issue

No response

@loynoir loynoir changed the title dynamic import foo.test.ts without export default {} leads to wrong type union merge dynamic import foo.test.ts without export default {} leads to wrong type union Mar 12, 2024
@RyanCavanaugh RyanCavanaugh added the Not a Defect This behavior is one of several equally-correct options label Mar 12, 2024
@RyanCavanaugh
Copy link
Member

You have two modules shapes here:

  • The one from a, which has a foo: number property
  • The one from a.test, which has either nothing, nothing, or a default: {} property

The "nothing" variants supertype the { foo: number } property, so the union import(...) | import(...) gets reduced to the more-general of the two (nothing)

@loynoir
Copy link
Author

loynoir commented Mar 13, 2024

@RyanCavanaugh

But if a.test don't have export statement, I think it should be something like Record<string, never>.

Because the only way to let ESM module have X property, is using export statement.

If there is zero export statement, it means a zero key object Record<string, never>.

Right?


Change to a.test.mts

  • still get typeof import("/path/to/reproduce/a.test")

  • instead of something like {foo: 42} | Record<string, never>.

@RyanCavanaugh
Copy link
Member

That would be a very bad type; given x: Record<string, never> you can write nonsense like Math.sin(x.foo) without error

@loynoir
Copy link
Author

loynoir commented Mar 13, 2024

https://github.com/typescript-eslint/typescript-eslint/blob/609a0003663e0409f1d277a3f7649d17b98b5cdb/packages/eslint-plugin/src/rules/ban-types.ts#L101

Quote recommendation from @typescript-eslint/ban-types

If you want a type meaning "empty object", you probably want Record<string, never> instead.

@fatcerberus
Copy link

The TypeScript maintainers have a... let's say... contentious relationship with the ban-types rule, specifically the part about it banning {}

Either way, Record<string, never> doesn't mean "empty object", regardless of what eslint recommends. It actually means "object where accessing any property throws an exception". Reading a property from an empty object doesn't give you a value of type never (i.e. an exception), it gives you undefined.

@loynoir
Copy link
Author

loynoir commented Mar 14, 2024

But, compared with {} which means any-alike, Record<string, never> seems to be yet the most correct type, to describe empty-object, maybe say always-zero-key-object, which describe an ESM module without any export statement.

@RyanCavanaugh
Copy link
Member

If you want a type meaning "empty object", you probably want Record<string, never> instead.

The typescript-eslint maintainers are uncharacteristically super wrong about this. { } is a valid type with a valid meaning and it's just wrong for them to ban it. Unfortunately we haven't had much luck changing their minds.

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Mar 17, 2024

👋 typescript-eslint maintainer here. The ban-types rule's default options were formed many years ago when TypeScript had less fleshed out rules around {}, object, and related. If there's evidence that the rule's options are now and/or always were wrong about something, we'd happily take an issue to improve them. The dev lead for TypeScript saying the rule is super wrong is pretty compelling evidence. 😄

Also looking at typescript-eslint/typescript-eslint#5018 (comment):

There's just a lot of confusion likely to come down the road on this specific ban in general, since we're likely to change the definition of NonNullable to T & { } in 4.8, and are already in 4.8 going to change the default narrowing rules such that a truthy narrowing of a value of an unconstrained type parameter becomes T & { }. Having typescript-eslint, out of the box, ban you from writing a type that TypeScript itself is inferring under totally normal operation, is awkward.

TypeScript did in fact ship the change to make type NonNullable<T> = T & {}; in #49119 - along with a host of other {} improvements. I'd wager that there's even more evidence we should revisit the ban.

Three related issues on our side:

For context: typescript-eslint is a separate maintenance team from TypeScript. Our opinions can sometimes fall out of sync with TypeScript best practices because we -like many independent open source projects- generally only take action in response to user prodding. We haven't been prodded about ban-types (that I know of) in quite a while. If there's anything ban-types or any other rule is doing that's out of sync with how TypeScript is meant to be, and TypeScript's handling of that rule's area has changed since the last time that rule was discussed, we'd happily take an issue prodding us to change. ❤️

@loynoir
Copy link
Author

loynoir commented Mar 17, 2024

As far as I see, maybe should split empty object type into another issue, if needed, as there are now two issue?

@JoshuaKGoldberg


The original issue is

Actual

ESM X.test which have zero export statement, is actually super type of ESM X which have at least one export statement

Expected

ESM X.test which have zero export statement, should be sub type of ESM X which have at least one export statement.

Because it is

  • zero-key-object-at-read

  • not zero-key-object-at-write, but consider as nop

> mod = await import('/tmp/empty-file.mjs')
> mod.x=1
1
> mod.x
undefined

@loynoir loynoir changed the title merge dynamic import foo.test.ts without export default {} leads to wrong type union ESM X.test which have zero export statement, should be sub type of ESM X which have at least one export statement Mar 17, 2024
@RyanCavanaugh
Copy link
Member

There's no type in TypeScript that means "a type that is guaranteed to be completely empty" because TypeScript doesn't have sealed/exact types

@typescript-bot
Copy link
Collaborator

This issue has been marked as "Not a Defect" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Not a Defect This behavior is one of several equally-correct options
Projects
None yet
Development

No branches or pull requests

5 participants