-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Proposal: Allow a name bound to a class value to be used in a type position to reference the class's instance type #36348
Comments
This all makes sense on the surface but I'm struggling to find the real use cases. Any time you write What are some reasons to write |
To answer your question, here are a couple of reasons I find the
I agree that for bare class expressions specifically, you could in many cases simply opt for const Foo = func(class { ... });
const foo: Foo = new Foo(...); Perhaps I didn't make it clear enough (apologies if so), but the intent is very much to make that possible. 🙂 TypeScript's current decorator syntax implementation doesn't support type-level changes to decorated classes, and given how radically the TC39 decorator proposal has diverged, I don't expect you guys (the TypeScript team) to be willing to change that. That's why one of the main motivations is class decoration; to serve as a workaround without encouraging further use of |
Aah, so this is what biting me with my mixins! I have the following code (Playground): class BaseClass {
}
type Constructor<T = {}> = new(...args: any[]) => T;
function getMyCoolClass<T extends Constructor>(base: T) {
return class extends base {
}
}
const MyCoolClass = getMyCoolClass(BaseClass)
interface MyInterface {
x: BaseClass
y: MyCoolClass
} And A possible solution for me is doing this: class MyBetterClass extends getMyCoolClass(BaseClass) {} But that seems a bit over the top... My current solution currently involves rewriting the code with a monkey-patching pattern instead (Playground): class BaseClass {
myFn() { return 4 };
}
type Constructor<T = {}> = new(...args: any[]) => T;
function setMyCoolClass<T extends Constructor<BaseClass>>(base: T) {
base.prototype.myFn = function () { return 3 };
}
class MyCoolClass extends BaseClass { }
setMyCoolClass(MyCoolClass)
interface MyInterface {
x: BaseClass
y: MyCoolClass
}
console.log(new BaseClass().myFn())
console.log((new MyCoolClass()).myFn()) It's kinda bad because I have to handle |
I think this would definitely help with mixins. |
@RyanCavanaugh This would obviously be a fairly niche feature, and one that would be difficult for the team to prioritise as a result. With that said, would the team be willing to accept a community PR for this? |
@treybrisbane the presence/absence of a PR doesn't change our feature acceptance criteria in either direction. This would be straightforward to implement; the question is rather whether or not it's a good idea for the language to behave this way. Looking at this again with fresh eyes, the bar is very high since it would effectively mean that adding any new type name to the global scope would start being a potential breaking change, since we would have to make global type names "win" over local value names for this change to itself not be an enormous breaking change. |
It's especially problematic in that regard since there's really no way of predicting what global type names are going to come into your program through |
@RyanCavanaugh Thanks for taking the time to look at this again. 🙂 You raise some very good points. With them in mind, I agree with your assessment that this is a pretty tough sell. I kinda wanna leave the issue open just on the off chance that it gains popularity and/or somehow becomes more feasible later, however given the implications of the change, I would totally understand if you want to decline and close it now. I'll leave that up to you to decide. Again, thanks for taking the time to look at this and write out your thoughts. 🙂 |
We use exports from`apollo-server-env` to access the Fetch API to avoid depending on running in specific environments. While environments like Cloudflare Workers and Deno expose a Fetch API globally, in Node environments you'd generally like to avoid polyfilling the global context. There are also differences between the APIs available in these environments that we may want to account for with our own types. Unfortunately,`apollo-fetch` uses `cross-fetch` as a global polyfill (see https://github.com/apollographql/apollo-fetch/blob/0a0b8df241960dfa72abf3bd179e9d936265a7da/packages/apollo-fetch/src/apollo-fetch.ts#L17), and expects having Fetch API types defined globally. Because we use `apollo-fetch` in our tests, we've long used a `global.d.ts` in `apollo-server-env` to make the exported types available globally without depending on the `dom` lib types (because most of the types in there don't really make sense in non-browser environments and can lead to accidental usage). As it turns out however, we also use `superagent` in our tests, and `@types/superagent` now specifies an explicit dependency on `dom` lib. As TypeScript makes any global type declarations available throughout a project, that means our tests now get the `dom` lib types merged into their environment, resulting in conflicts with the types in `apollo-server-env`'s `global.d.ts`. Forcing anyone depending on packages like `@types/superagent` to have their environment extended with the `dom` lib types isn't great, and there are existing discussions about the implications of this for that package specifically (see DefinitelyTyped/DefinitelyTyped#41425). There is also a promising proposal for a `strictEnvironment` option in TypeScript that would help avoid these situations (see https://gist.github.com/RyanCavanaugh/702ebd1ca2fc060e58e634b4e30c1c1c). For now, I've solved this by reluctantly adding `dom` to `tsconfig.test.base.json`. We should be able to remove this when our dependencies get fixed or if `strictEnvironment` gets implemented. I also fixed some typing issues that ironically illustrate the danger of having these `dom` types available. Because the mocked version of `apollo-server-env` exported classes through a `const` declaration, that didn't actually export them as types, just as values (see microsoft/TypeScript#36348). But because we have similarly named types defined in `dom`, those were used instead. Using named exports in the `apollo-server-env` mock avoids this.
We use exports from`apollo-server-env` to access the Fetch API to avoid depending on running in specific environments. While environments like Cloudflare Workers and Deno expose a Fetch API globally, in Node environments you'd generally like to avoid polyfilling the global context. There are also differences between the APIs available in these environments that we may want to account for with our own types. Unfortunately,`apollo-fetch` uses `cross-fetch` as a global polyfill (see https://github.com/apollographql/apollo-fetch/blob/0a0b8df241960dfa72abf3bd179e9d936265a7da/packages/apollo-fetch/src/apollo-fetch.ts#L17), and expects having Fetch API types defined globally. Because we use `apollo-fetch` in our tests, we've long used a `global.d.ts` in `apollo-server-env` to make the exported types available globally without depending on the `dom` lib types (because most of the types in there don't really make sense in non-browser environments and can lead to accidental usage). As it turns out however, we also use `superagent` in our tests, and `@types/superagent` now specifies an explicit dependency on `dom` lib. As TypeScript makes any global type declarations available throughout a project, that means our tests now get the `dom` lib types merged into their environment, resulting in conflicts with the types in `apollo-server-env`'s `global.d.ts`. Forcing anyone depending on packages like `@types/superagent` to have their environment extended with the `dom` lib types isn't great, and there are existing discussions about the implications of this for that package specifically (see DefinitelyTyped/DefinitelyTyped#41425). There is also a promising proposal for a `strictEnvironment` option in TypeScript that would help avoid these situations (see https://gist.github.com/RyanCavanaugh/702ebd1ca2fc060e58e634b4e30c1c1c). For now, I've solved this by reluctantly adding `dom` to `tsconfig.test.base.json`. We should be able to remove this when our dependencies get fixed or if `strictEnvironment` gets implemented. I also fixed some typing issues that ironically illustrate the danger of having these `dom` types available. Because the mocked version of `apollo-server-env` exported classes through a `const` declaration, that didn't actually export them as types, just as values (see microsoft/TypeScript#36348). But because we have similarly named types defined in `dom`, those were used instead. Using named exports in the `apollo-server-env` mock avoids this.
Upgrading to TS 4 is generally useful, but it also lets use combine `composite` with `noEmit`, which lets us actually use `tsconfig.test.json`. This PR copies a few things we've done recently in the Federation repo. - Upgrade TypeScript, Jest, and Codecov. - Drop jest-cucumber dep (hasn't been used for a while). - Update VSCode task to use the "typescript" type. - Set enablePromptUseWorkspaceTsdk like in apollographql/federation#728 - Stop using moduleNameMapper in jest config, which seemed to create weird issues. Instead, just make sure that `npm run compile` happens before you run tests (at least when run via `npm test`) via a `pretest` script. - Fix a lot of compilation errors (mostly caused by actually typechecking tests, though some from the upgrade). Most of this is pretty localized, but some required more work (eg, replacing a `Map` that was being treated as a `KeyValueCache` with an actual `KeyValueCache` implementation). - I found the moduleNameMapper pulling in a top-level `__mocks__` to be confusing and also led to errors about trying to import files from outside a project. In fact in general I found `__mocks__` to be confusing. So: - I replaced our use of a custom `Date` mocker plus `jest.useFakeTimers` with `@sinonjs/fake-timers` (which does both and is what jest's is built on) - Replaced `__mocks__/apollo-server-env.ts` with a file that doesn't have a special `__mocks__` name - Did the same for the ioredis mock, and loaded it into `jest.mock` explicitly rather than implicitly - The thing where we imported a function that defined a jest test suite from the `__tests__` of `apollo-server-caching` was sort of weird already, and importing from a project that used `noEmit` didn't seem to work at all here. So I changed the testing function to just be a normal non-Jest-specific function exported from `apollo-server-caching` that can be used in Jest tests (or non-Jest tests for that matter) with a bit of preamble. (I changed its name too so it's obviously a different function.) Because closing the cache is now not part of the test, we didn't need `TestableKeyValueCache` any more so I dropped it. - isDirectiveDefined isn't exported and is now always called with an array, so remove code that runs if it doesn't get one and tests that that code works. - RESTDataSource used to document and test that you can override `baseURL` in a subclass with a getter. This is illegal in TS4 so we don't officially support it any more. Additionally, this PR removes global types from `apollo-server-env` (the `global.d.ts` file). Thanks to @martijnwalraven for figuring this out and writing the following: We use exports from `apollo-server-env` to access the Fetch API to avoid depending on running in specific environments. While environments like Cloudflare Workers and Deno expose a Fetch API globally, in Node environments you'd generally like to avoid polyfilling the global context. There are also differences between the APIs available in these environments that we may want to account for with our own types. Unfortunately,`apollo-fetch` uses `cross-fetch` as a global polyfill (see https://github.com/apollographql/apollo-fetch/blob/0a0b8df241960dfa72abf3bd179e9d936265a7da/packages/apollo-fetch/src/apollo-fetch.ts#L17), and expects having Fetch API types defined globally. Because we use `apollo-fetch` in our tests, we've long used a `global.d.ts` in `apollo-server-env` to make the exported types available globally without depending on the `dom` lib types (because most of the types in there don't really make sense in non-browser environments and can lead to accidental usage). As it turns out however, we also use `superagent` in our tests, and `@types/superagent` now specifies an explicit dependency on `dom` lib. As TypeScript makes any global type declarations available throughout a project, that means our tests now get the `dom` lib types merged into their environment, resulting in conflicts with the types in `apollo-server-env`'s `global.d.ts`. Forcing anyone depending on packages like `@types/superagent` to have their environment extended with the `dom` lib types isn't great, and there are existing discussions about the implications of this for that package specifically (see DefinitelyTyped/DefinitelyTyped#41425). There is also a promising proposal for a `strictEnvironment` option in TypeScript that would help avoid these situations (see https://gist.github.com/RyanCavanaugh/702ebd1ca2fc060e58e634b4e30c1c1c). For now, I've solved this by reluctantly adding `dom` to `tsconfig.test.base.json`. We should be able to remove this when our dependencies get fixed or if `strictEnvironment` gets implemented. I also fixed some typing issues that ironically illustrate the danger of having these `dom` types available. Because the mocked version of `apollo-server-env` exported classes through a `const` declaration, that didn't actually export them as types, just as values (see microsoft/TypeScript#36348). But because we have similarly named types defined in `dom`, those were used instead. Using named exports in the `apollo-server-env` mock avoids this.
Upgrading to TS 4 is generally useful, but it also lets use combine `composite` with `noEmit`, which lets us actually use `tsconfig.test.json`. This PR copies a few things we've done recently in the Federation repo. - Upgrade TypeScript, Jest, and Codecov. - Drop jest-cucumber dep (hasn't been used for a while). - Update VSCode task to use the "typescript" type. - Set enablePromptUseWorkspaceTsdk like in apollographql/federation#728 - Stop using moduleNameMapper in jest config, which seemed to create weird issues. Instead, just make sure that `npm run compile` happens before you run tests (at least when run via `npm test`) via a `pretest` script. - Fix a lot of compilation errors (mostly caused by actually typechecking tests, though some from the upgrade). Most of this is pretty localized, but some required more work (eg, replacing a `Map` that was being treated as a `KeyValueCache` with an actual `KeyValueCache` implementation). - I found the moduleNameMapper pulling in a top-level `__mocks__` to be confusing and also led to errors about trying to import files from outside a project. In fact in general I found `__mocks__` to be confusing. So: - I replaced our use of a custom `Date` mocker plus `jest.useFakeTimers` with `@sinonjs/fake-timers` (which does both and is what jest's is built on) - Replaced `__mocks__/apollo-server-env.ts` with a file that doesn't have a special `__mocks__` name - Did the same for the ioredis mock, and loaded it into `jest.mock` explicitly rather than implicitly - The thing where we imported a function that defined a jest test suite from the `__tests__` of `apollo-server-caching` was sort of weird already, and importing from a project that used `noEmit` didn't seem to work at all here. So I changed the testing function to just be a normal non-Jest-specific function exported from `apollo-server-caching` that can be used in Jest tests (or non-Jest tests for that matter) with a bit of preamble. (I changed its name too so it's obviously a different function.) Because closing the cache is now not part of the test, we didn't need `TestableKeyValueCache` any more so I dropped it. - isDirectiveDefined isn't exported and is now always called with an array, so remove code that runs if it doesn't get one and tests that that code works. - RESTDataSource used to document and test that you can override `baseURL` in a subclass with a getter. This is illegal in TS4 so we don't officially support it any more. Additionally, this PR removes global types from `apollo-server-env` (the `global.d.ts` file). Thanks to @martijnwalraven for figuring this out and writing the following: We use exports from `apollo-server-env` to access the Fetch API to avoid depending on running in specific environments. While environments like Cloudflare Workers and Deno expose a Fetch API globally, in Node environments you'd generally like to avoid polyfilling the global context. There are also differences between the APIs available in these environments that we may want to account for with our own types. Unfortunately,`apollo-fetch` uses `cross-fetch` as a global polyfill (see https://github.com/apollographql/apollo-fetch/blob/0a0b8df241960dfa72abf3bd179e9d936265a7da/packages/apollo-fetch/src/apollo-fetch.ts#L17), and expects having Fetch API types defined globally. Because we use `apollo-fetch` in our tests, we've long used a `global.d.ts` in `apollo-server-env` to make the exported types available globally without depending on the `dom` lib types (because most of the types in there don't really make sense in non-browser environments and can lead to accidental usage). As it turns out however, we also use `superagent` in our tests, and `@types/superagent` now specifies an explicit dependency on `dom` lib. As TypeScript makes any global type declarations available throughout a project, that means our tests now get the `dom` lib types merged into their environment, resulting in conflicts with the types in `apollo-server-env`'s `global.d.ts`. Forcing anyone depending on packages like `@types/superagent` to have their environment extended with the `dom` lib types isn't great, and there are existing discussions about the implications of this for that package specifically (see DefinitelyTyped/DefinitelyTyped#41425). There is also a promising proposal for a `strictEnvironment` option in TypeScript that would help avoid these situations (see https://gist.github.com/RyanCavanaugh/702ebd1ca2fc060e58e634b4e30c1c1c). For now, I've solved this by reluctantly adding `dom` to `tsconfig.test.base.json`. We should be able to remove this when our dependencies get fixed or if `strictEnvironment` gets implemented. I also fixed some typing issues that ironically illustrate the danger of having these `dom` types available. Because the mocked version of `apollo-server-env` exported classes through a `const` declaration, that didn't actually export them as types, just as values (see microsoft/TypeScript#36348). But because we have similarly named types defined in `dom`, those were used instead. Using named exports in the `apollo-server-env` mock avoids this. Fixes #5119.
Upgrading to TS 4 is generally useful, but it also lets use combine `composite` with `noEmit`, which lets us actually use `tsconfig.test.json`. This PR copies a few things we've done recently in the Federation repo. - Upgrade TypeScript, Jest, and Codecov. - Drop jest-cucumber dep (hasn't been used for a while). - Update VSCode task to use the "typescript" type. - Set enablePromptUseWorkspaceTsdk like in apollographql/federation#728 - Stop using moduleNameMapper in jest config, which seemed to create weird issues. Instead, just make sure that `npm run compile` happens before you run tests (at least when run via `npm test`) via a `pretest` script. - Fix a lot of compilation errors (mostly caused by actually typechecking tests, though some from the upgrade). Most of this is pretty localized, but some required more work (eg, replacing a `Map` that was being treated as a `KeyValueCache` with an actual `KeyValueCache` implementation). - I found the moduleNameMapper pulling in a top-level `__mocks__` to be confusing and also led to errors about trying to import files from outside a project. In fact in general I found `__mocks__` to be confusing. So: - I replaced our use of a custom `Date` mocker plus `jest.useFakeTimers` with `@sinonjs/fake-timers` (which does both and is what jest's is built on) - Replaced `__mocks__/apollo-server-env.ts` with a file that doesn't have a special `__mocks__` name - Did the same for the ioredis mock, and loaded it into `jest.mock` explicitly rather than implicitly - The thing where we imported a function that defined a jest test suite from the `__tests__` of `apollo-server-caching` was sort of weird already, and importing from a project that used `noEmit` didn't seem to work at all here. So I changed the testing function to just be a normal non-Jest-specific function exported from `apollo-server-caching` that can be used in Jest tests (or non-Jest tests for that matter) with a bit of preamble. (I changed its name too so it's obviously a different function.) Because closing the cache is now not part of the test, we didn't need `TestableKeyValueCache` any more so I dropped it. - isDirectiveDefined isn't exported and is now always called with an array, so remove code that runs if it doesn't get one and tests that that code works. - RESTDataSource used to document and test that you can override `baseURL` in a subclass with a getter. This is illegal in TS4 so we don't officially support it any more. Additionally, this PR removes global types from `apollo-server-env` (the `global.d.ts` file). Thanks to @martijnwalraven for figuring this out and writing the following: We use exports from `apollo-server-env` to access the Fetch API to avoid depending on running in specific environments. While environments like Cloudflare Workers and Deno expose a Fetch API globally, in Node environments you'd generally like to avoid polyfilling the global context. There are also differences between the APIs available in these environments that we may want to account for with our own types. Unfortunately,`apollo-fetch` uses `cross-fetch` as a global polyfill (see https://github.com/apollographql/apollo-fetch/blob/0a0b8df241960dfa72abf3bd179e9d936265a7da/packages/apollo-fetch/src/apollo-fetch.ts#L17), and expects having Fetch API types defined globally. Because we use `apollo-fetch` in our tests, we've long used a `global.d.ts` in `apollo-server-env` to make the exported types available globally without depending on the `dom` lib types (because most of the types in there don't really make sense in non-browser environments and can lead to accidental usage). As it turns out however, we also use `superagent` in our tests, and `@types/superagent` now specifies an explicit dependency on `dom` lib. As TypeScript makes any global type declarations available throughout a project, that means our tests now get the `dom` lib types merged into their environment, resulting in conflicts with the types in `apollo-server-env`'s `global.d.ts`. Forcing anyone depending on packages like `@types/superagent` to have their environment extended with the `dom` lib types isn't great, and there are existing discussions about the implications of this for that package specifically (see DefinitelyTyped/DefinitelyTyped#41425). There is also a promising proposal for a `strictEnvironment` option in TypeScript that would help avoid these situations (see https://gist.github.com/RyanCavanaugh/702ebd1ca2fc060e58e634b4e30c1c1c). For now, I've solved this by reluctantly adding `dom` to `tsconfig.test.base.json`. We should be able to remove this when our dependencies get fixed or if `strictEnvironment` gets implemented. I also fixed some typing issues that ironically illustrate the danger of having these `dom` types available. Because the mocked version of `apollo-server-env` exported classes through a `const` declaration, that didn't actually export them as types, just as values (see microsoft/TypeScript#36348). But because we have similarly named types defined in `dom`, those were used instead. Using named exports in the `apollo-server-env` mock avoids this. Fixes #5119.
Upgrading to TS 4 is generally useful, but it also lets use combine `composite` with `noEmit`, which lets us actually use `tsconfig.test.json`. This PR copies a few things we've done recently in the Federation repo. - Upgrade TypeScript, Jest, and Codecov. - Drop jest-cucumber dep (hasn't been used for a while). - Update VSCode task to use the "typescript" type. - Set enablePromptUseWorkspaceTsdk like in apollographql/federation#728 - Stop using moduleNameMapper in jest config, which seemed to create weird issues. Instead, just make sure that `npm run compile` happens before you run tests (at least when run via `npm test`) via a `pretest` script. - Fix a lot of compilation errors (mostly caused by actually typechecking tests, though some from the upgrade). Most of this is pretty localized, but some required more work (eg, replacing a `Map` that was being treated as a `KeyValueCache` with an actual `KeyValueCache` implementation). - I found the moduleNameMapper pulling in a top-level `__mocks__` to be confusing and also led to errors about trying to import files from outside a project. In fact in general I found `__mocks__` to be confusing. So: - I replaced our use of a custom `Date` mocker plus `jest.useFakeTimers` with `@sinonjs/fake-timers` (which does both and is what jest's is built on) - Replaced `__mocks__/apollo-server-env.ts` with a file that doesn't have a special `__mocks__` name - Did the same for the ioredis mock, and loaded it into `jest.mock` explicitly rather than implicitly - The thing where we imported a function that defined a jest test suite from the `__tests__` of `apollo-server-caching` was sort of weird already, and importing from a project that used `noEmit` didn't seem to work at all here. So I changed the testing function to just be a normal non-Jest-specific function exported from `apollo-server-caching` that can be used in Jest tests (or non-Jest tests for that matter) with a bit of preamble. (I changed its name too so it's obviously a different function.) Because closing the cache is now not part of the test, we didn't need `TestableKeyValueCache` any more so I dropped it. - isDirectiveDefined isn't exported and is now always called with an array, so remove code that runs if it doesn't get one and tests that that code works. - RESTDataSource used to document and test that you can override `baseURL` in a subclass with a getter. This is illegal in TS4 so we don't officially support it any more. Additionally, this PR removes global types from `apollo-server-env` (the `global.d.ts` file). Thanks to @martijnwalraven for figuring this out and writing the following: We use exports from `apollo-server-env` to access the Fetch API to avoid depending on running in specific environments. While environments like Cloudflare Workers and Deno expose a Fetch API globally, in Node environments you'd generally like to avoid polyfilling the global context. There are also differences between the APIs available in these environments that we may want to account for with our own types. Unfortunately,`apollo-fetch` uses `cross-fetch` as a global polyfill (see https://github.com/apollographql/apollo-fetch/blob/0a0b8df241960dfa72abf3bd179e9d936265a7da/packages/apollo-fetch/src/apollo-fetch.ts#L17), and expects having Fetch API types defined globally. Because we use `apollo-fetch` in our tests, we've long used a `global.d.ts` in `apollo-server-env` to make the exported types available globally without depending on the `dom` lib types (because most of the types in there don't really make sense in non-browser environments and can lead to accidental usage). As it turns out however, we also use `superagent` in our tests, and `@types/superagent` now specifies an explicit dependency on `dom` lib. As TypeScript makes any global type declarations available throughout a project, that means our tests now get the `dom` lib types merged into their environment, resulting in conflicts with the types in `apollo-server-env`'s `global.d.ts`. Forcing anyone depending on packages like `@types/superagent` to have their environment extended with the `dom` lib types isn't great, and there are existing discussions about the implications of this for that package specifically (see DefinitelyTyped/DefinitelyTyped#41425). There is also a promising proposal for a `strictEnvironment` option in TypeScript that would help avoid these situations (see https://gist.github.com/RyanCavanaugh/702ebd1ca2fc060e58e634b4e30c1c1c). For now, I've solved this by reluctantly adding `dom` to `tsconfig.test.base.json`. We should be able to remove this when our dependencies get fixed or if `strictEnvironment` gets implemented. I also fixed some typing issues that ironically illustrate the danger of having these `dom` types available. Because the mocked version of `apollo-server-env` exported classes through a `const` declaration, that didn't actually export them as types, just as values (see microsoft/TypeScript#36348). But because we have similarly named types defined in `dom`, those were used instead. Using named exports in the `apollo-server-env` mock avoids this. Fixes #5119.
Upgrading to TS 4 is generally useful, but it also lets use combine `composite` with `noEmit`, which lets us actually use `tsconfig.test.json`. This PR copies a few things we've done recently in the Federation repo. - Upgrade TypeScript, Jest, and Codecov. - Drop jest-cucumber dep (hasn't been used for a while). - Update VSCode task to use the "typescript" type. - Set enablePromptUseWorkspaceTsdk like in apollographql/federation#728 - Stop using moduleNameMapper in jest config, which seemed to create weird issues. Instead, just make sure that `npm run compile` happens before you run tests (at least when run via `npm test`) via a `pretest` script. - Fix a lot of compilation errors (mostly caused by actually typechecking tests, though some from the upgrade). Most of this is pretty localized, but some required more work (eg, replacing a `Map` that was being treated as a `KeyValueCache` with an actual `KeyValueCache` implementation). - I found the moduleNameMapper pulling in a top-level `__mocks__` to be confusing and also led to errors about trying to import files from outside a project. In fact in general I found `__mocks__` to be confusing. So: - I replaced our use of a custom `Date` mocker plus `jest.useFakeTimers` with `@sinonjs/fake-timers` (which does both and is what jest's is built on) - Replaced `__mocks__/apollo-server-env.ts` with a file that doesn't have a special `__mocks__` name - Did the same for the ioredis mock, and loaded it into `jest.mock` explicitly rather than implicitly - The thing where we imported a function that defined a jest test suite from the `__tests__` of `apollo-server-caching` was sort of weird already, and importing from a project that used `noEmit` didn't seem to work at all here. So I changed the testing function to just be a normal non-Jest-specific function exported from `apollo-server-caching` that can be used in Jest tests (or non-Jest tests for that matter) with a bit of preamble. (I changed its name too so it's obviously a different function.) Because closing the cache is now not part of the test, we didn't need `TestableKeyValueCache` any more so I dropped it. - isDirectiveDefined isn't exported and is now always called with an array, so remove code that runs if it doesn't get one and tests that that code works. - RESTDataSource used to document and test that you can override `baseURL` in a subclass with a getter. This is illegal in TS4 so we don't officially support it any more. Additionally, this PR removes global types from `apollo-server-env` (the `global.d.ts` file). Thanks to @martijnwalraven for figuring this out and writing the following: We use exports from `apollo-server-env` to access the Fetch API to avoid depending on running in specific environments. While environments like Cloudflare Workers and Deno expose a Fetch API globally, in Node environments you'd generally like to avoid polyfilling the global context. There are also differences between the APIs available in these environments that we may want to account for with our own types. Unfortunately,`apollo-fetch` uses `cross-fetch` as a global polyfill (see https://github.com/apollographql/apollo-fetch/blob/0a0b8df241960dfa72abf3bd179e9d936265a7da/packages/apollo-fetch/src/apollo-fetch.ts#L17), and expects having Fetch API types defined globally. Because we use `apollo-fetch` in our tests, we've long used a `global.d.ts` in `apollo-server-env` to make the exported types available globally without depending on the `dom` lib types (because most of the types in there don't really make sense in non-browser environments and can lead to accidental usage). As it turns out however, we also use `superagent` in our tests, and `@types/superagent` now specifies an explicit dependency on `dom` lib. As TypeScript makes any global type declarations available throughout a project, that means our tests now get the `dom` lib types merged into their environment, resulting in conflicts with the types in `apollo-server-env`'s `global.d.ts`. Forcing anyone depending on packages like `@types/superagent` to have their environment extended with the `dom` lib types isn't great, and there are existing discussions about the implications of this for that package specifically (see DefinitelyTyped/DefinitelyTyped#41425). There is also a promising proposal for a `strictEnvironment` option in TypeScript that would help avoid these situations (see https://gist.github.com/RyanCavanaugh/702ebd1ca2fc060e58e634b4e30c1c1c). For now, I've solved this by reluctantly adding `dom` to `tsconfig.test.base.json`. We should be able to remove this when our dependencies get fixed or if `strictEnvironment` gets implemented. I also fixed some typing issues that ironically illustrate the danger of having these `dom` types available. Because the mocked version of `apollo-server-env` exported classes through a `const` declaration, that didn't actually export them as types, just as values (see microsoft/TypeScript#36348). But because we have similarly named types defined in `dom`, those were used instead. Using named exports in the `apollo-server-env` mock avoids this. Fixes #5119.
@RyanCavanaugh you need this if you use mixins I use mixins a lot—composition is better than inheritance most of the times—and cannot use them as types anymore? At the end of the day, they are perfectly fine classes but they can't be used as types? So, this is not just an edge use case but it took me also half a day to understand what's wrong with my code. First, I had to refactor half a day to understand that mixins are the culprit and then, I had to find this thread to give the confusion a name. I'd say next to some missing essential feature we add some unintuitive behavior which adds a lot confusion and a so-so dev experience to the mix. Using classes as types/interfaces is one of the killer features of TS. Check the last two lines for an example and to see the difference: https://stackblitz.com/edit/typescript-1czz7l?file=index.ts |
Proposal: Allow a name bound to a class value to be used in a type position to reference the class's instance type
What?
In TypeScript, when a class is created and bound to a name via a class declaration, the name can be used in both value and type positions. In contrast, when a class is bound to a name via
const
,let
, orvar
, the name can only be used in value positions.I propose that when a class is bound to a name via
const
,let
, orvar
, the name should be usable in both value and type positions, and that when such a name is used in a type position, it be interpreted as the instance type of the bound class value.More formally:
const
,let
, orvar
statement, andnew (...args: any[]) => any
, andany
Examples of proposed behaviour
Why?
Unlike a class declaration, a class value requires a separate type declaration to expose the class's instance type
For class declarations, we can simply use the name of the class in a type position to reference its instance type:
But for class values, a separate type declaration is required:
Requiring a separate type declaration has a few issues:
With this proposal however, we wouldn't need a separate type declaration, and all of the following would just work:
There is currently no way to access the generic instance type of a generic class value
None of the following work:
With this proposal however, we could simply use the name of the class value in a type position to reference its generic instance type:
It enables a potential workaround for #4881
This doesn't work:
But with this proposal, we could do this:
While this wouldn't be type support for actual decorators, it would at least provide a means of class decoration that's reflected at the type level.
Flow supports this (there is prior art)
The following all work in Flow:
While feature parity with Flow is obviously not one of TypeScript's goals, Flow supporting this behaviour means that there's at least a precedent for it.
Why not?
It may be a breaking change
Depending on the implementation approach, this may be a breaking change.
For example, if this proposal were to be implemented by having the compiler automagically generate a separate type name whenever it encountered a ClassValueName, the generated type name may clash with an existing type name and break that currently working code.
On the other hand, if it were possible to implement this proposal using some kind of fallback mechanism (such as preferring any existing type name over using a ClassValueName in a type position, for instance), then the change would be backwards compatible.
It is currently unclear whether or not the proposed change can be implemented in a backwards compatible manner.
It "muddies" the separation between the value and type namespaces
TypeScript maintains separate namespaces for values and types. While this separation is fairly core to the language, there are already several exceptions in the form of class declarations, enums, and namespaces.
This proposal would introduce a new exception to that separation. This exception is particularly notable, as it would mark the first time a
const
,let
, orvar
-declared name would be permitted in a type position. This is in contrast to all current exceptions, which each have construct-specific declaration syntax that differentiates them from standard variable declarations.This proposal's "muddying" of the separation between the value and type namespaces may be confusing and/or surprising for both new and existing TypeScript users.
Next steps
Checklist
My suggestion meets these guidelines:
The text was updated successfully, but these errors were encountered: