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

3.8 RC Regression? "Individual declarations in merged declaration" #36684

Closed
garrettm opened this issue Feb 7, 2020 · 21 comments
Closed

3.8 RC Regression? "Individual declarations in merged declaration" #36684

garrettm opened this issue Feb 7, 2020 · 21 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@garrettm
Copy link

garrettm commented Feb 7, 2020

TypeScript Version: 3.8.1-rc and nightly (3.8.0-dev.20200207)

Search Terms:
Individual declarations in merged declaration

Code
Two files:

// ns.ts
export type Type = {greet: string}

export function hello(input: Type) {
  console.log(input.greet)
}
// use.ts
import * as Greeting from './ns'

export type Greeting = Greeting.Type
export {Greeting}

Expected behavior:
Expected that this compiles and users can refer to Greeting as both the type {greet: string} and the namespace of functions/data (just hello in this case). In versions 3.7.5 and 3.8.0-beta, this was OK.

Actual behavior:
3.8.1-rc and current nightly produce these errors:

use.ts:1:13 - error TS2395: Individual declarations in merged declaration 'Greeting' must be all exported or all local.

1 import * as Greeting from './ns'
              ~~~~~~~~

use.ts:3:13 - error TS2395: Individual declarations in merged declaration 'Greeting' must be all exported or all local.

3 export type Greeting = Greeting.Type
              ~~~~~~~~
@DanielRosenwasser
Copy link
Member

@andrewbranch I thought this was a break in 3.7

@DanielRosenwasser
Copy link
Member

We discussed it in #33422 and I'm pretty sure the relevant issue was #31231.

@andrewbranch
Copy link
Member

@DanielRosenwasser it looks like #31231 simply failed to handle the combination of namespace import + type alias... 😱

@andrewbranch
Copy link
Member

I guess we accidentally fixed that case, which we had previously accidentally left unfixed, sometime after the beta? Possibly with #36237? Sorry @DanielRosenwasser, I should have recognized #36237 as a breaking change 😕

@andrewbranch andrewbranch added the Working as Intended The behavior described is the intended behavior; this is not a bug label Feb 13, 2020
@garrettm
Copy link
Author

@andrewbranch @DanielRosenwasser this is working as intended? So, I'll need to change my code to upgrade to 3.8+?

@DanielRosenwasser
Copy link
Member

Yes, I believe so. It's something I wish we could've communicated sooner, but the code never should have compiled.

@garrettm
Copy link
Author

garrettm commented Feb 13, 2020

Okay, thanks. For the record, this pattern can be really useful in terms of exposing a type + a set of functions on that type.

You can do this with an explicit namespace in a file, but I thought that was not encouraged anymore. Like this:

export type Greeting = {greet: string}
export namespace Greeting {
  export function hello(input: Greeting) {
    console.log(input.greet)
  }
}

These seem isomorphic and it seems weird to allow one and not the other.

@andrewbranch
Copy link
Member

@garrettm the issue isn’t with what will merge as exports, it’s that imports are never allowed to merge with local declarations. You could rewrite your original example as

// use.ts
import * as ns from './ns'

export type Greeting = ns.Type
export { ns as Greeting }

and that would be fine.

@garrettm
Copy link
Author

@andrewbranch thanks! That worked.

@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@danprince
Copy link

danprince commented Apr 7, 2020

@garrettm @andrewbranch Do you have an example of that rewritten form working in the wild?

It seems like the syntax is fine but I can't actually import the value of ns from use.ts.

// example-1.ts
import { Greeting } from "./use.ts";

let greeting: Greeting = { greet: "hi" };

Greeting.hello(greeting);
// 'Greeting' only refers to a type, but is being used as a value here.

Oddly enough, it seems to work when I import the entire module with a wildcard.

// example-2.ts
import * as Use from "./use.ts";

let greeting: Use.Greeting = { greet: "hi" };

Use.Greeting.hello(greeting);

I had been using the following workaround:

// use.ts

import * as ns from "./ns";

export const Greeting = ns;
export type Greeting = ns.Greeting;

But I've just discovered that it breaks tree shaking for Rollup (and presumably other bundlers). Anything that imports use.ts will end up with a full copy of the ns.ts module, irrespective of which exports they're actually using.

The important difference seems to be that tree shaking only works when the values are re-exported directly.

// works with tree shaking, not with typescript
export { ns as Greeting };

// works with typescript, not with tree shaking
export const Greeting = ns.Greeting;

This is possibly a separate issue to the original report, but I'm not sure. Here's a repro for anyone who can dig further than I can https://github.com/danprince/ts-export-types-bug

@garrettm
Copy link
Author

Hmm curious, I'm not having any luck with the latest non-beta TSC with @andrewbranch's workaround anymore either. Maybe I had a slightly different setup, or maybe this changed in a 3.8.x?

Your workaround does work for me @danprince

It would be nice if this was more ergonomic in general in typescript

@garrettm
Copy link
Author

garrettm commented Apr 23, 2020

One downside of @danprince's workaround is that you can't export types from the namespace, you get this error when you try to use it:
'Editor' only refers to a type, but is being used as a namespace here.

@andrewbranch
Copy link
Member

@danprince sorry I missed your comment earlier; that looks like it’s probably a bug. The Greeting symbol in export { ns as Greeting } is clearly not merging with the Greeting symbol in export type Greeting = ns.Type. I’m now second-guessing myself as to whether they’re supposed to merge, but the correct behavior is either

  1. Give a duplicate identifier error at both exports, or
  2. be able to reference both the type and the namespace meanings when you import it.

You’re seeing neither of those, which I’m pretty confident is wrong.

@andrewbranch
Copy link
Member

It does seem like this behavior goes all the way back to TypeScript 1.x though, so I’m not sure it’s a candidate for a last-minute 3.9 fix 😄

@garrettm
Copy link
Author

garrettm commented Apr 23, 2020

Something that makes this type of thing easy to do would be super appreciated, from this user's perspective :)

I haven't been able to find a good workaround for this at the moment

@danprince
Copy link

Hey @andrewbranch! Thanks for clarifying here. Is this behaviour going to be considered for a post 3.9 release?

Spending a lot of time writing Rectangle.Rectangle and Vector.Vector at the moment and it would be great to chop them down, or at least to know it's a wontfix.

@andrewbranch
Copy link
Member

It’s definitely not going to be included in a 3.9.x release, as the window for anything besides critical fixes closed a while ago. But I’m pretty sure the “bug” here is that we don’t give an error sooner:

export type Greeting = {};
export { ns as Greeting };

These two exports shouldn’t be allowed to coexist. In a way, it looks like they already don’t coexist because the alias meaning is unreferenceable from an import, but we should be giving you an error here on the exports. So I don’t think we’re really looking at any work that will give you the results you’re looking for. If it’s important to you, you should open a new issue, but I’d suggest including some real-world code that demonstrates why this is important. From my perspective here, it looks a little bit like an aesthetic problem that a slight reorganization of file structure or a slight reimagining of names would solve, and it’s hard to understand why it matters with examples like Greeting.

@garrettm
Copy link
Author

@andrewbranch here's a slightly more real code example that's basically a real world example with details stripped out and trimmed down a lot for length.

Users of this file want to import * as Animal from './animal' which works for everything except the exported type Animal, which has to be referred to as Animal.Animal. I'm not aware of a way to keep the nice namespacing of import * as and also refer to the Animal type itself easily.
One workaround is:

import * as A from './animal'
import {Animal} from './animal'
But this is inconvenient + less clear

// animal.ts
export type Cat = {type: 'cat', remainingLives: string}
export type Dog = {type: 'dog', breed: string}
export type Animal = Cat | Dog

export function greet(animal: Animal) {
  switch (animal.type) {
    case 'cat': return 'meow'
    case 'dog': return 'bark'
  }
}

export function remainingLives(animal: Animal) {
  switch (animal.type) {
    case 'cat': return animal.remainingLives
    case 'dog': return undefined
  }
}

export function isCat(animal: Animal): animal is Cat {
  return animal.type === 'cat'
}

@cruhl
Copy link

cruhl commented Aug 17, 2020

@garrettm This issue might help: #39865

@cruhl
Copy link

cruhl commented Aug 17, 2020

@andrewbranch I can also confirm my codebase is littered with NamespaceAndType.NamespaceAndType like @danprince mentioned...

Spending a lot of time writing Rectangle.Rectangle and Vector.Vector at the moment and it would be great to chop them down, or at least to know it's a wontfix.

Every single one of these files is imported like import * as Blah from "./Blah";...

image

...which means I write Asset.Asset, Color.Color, Company.Company, etc. a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

6 participants