Skip to content
This repository has been archived by the owner on Mar 24, 2022. It is now read-only.

fix: Use simulated opaque values for TreeAdapterTypeMap #42

Draft
wants to merge 182 commits into
base: master
Choose a base branch
from

Conversation

fb55
Copy link

@fb55 fb55 commented Dec 28, 2021

Opening this to have a base of discussion for #38.

This produces all of the expected type errors — see the failing tests.

fb55 and others added 22 commits December 9, 2021 13:04
Bumps [typescript](https://github.com/Microsoft/TypeScript) from 4.5.2 to 4.5.3.
- [Release notes](https://github.com/Microsoft/TypeScript/releases)
- [Commits](https://github.com/Microsoft/TypeScript/commits)

---
updated-dependencies:
- dependency-name: typescript
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [jest](https://github.com/facebook/jest) from 27.4.3 to 27.4.4.
- [Release notes](https://github.com/facebook/jest/releases)
- [Changelog](https://github.com/facebook/jest/blob/main/CHANGELOG.md)
- [Commits](jestjs/jest@v27.4.3...v27.4.4)

---
updated-dependencies:
- dependency-name: jest
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
)

Bumps [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser) from 5.6.0 to 5.7.0.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/parser/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.7.0/packages/parser)

---
updated-dependencies:
- dependency-name: "@typescript-eslint/parser"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [typescript](https://github.com/Microsoft/TypeScript) from 4.5.3 to 4.5.4.
- [Release notes](https://github.com/Microsoft/TypeScript/releases)
- [Commits](microsoft/TypeScript@v4.5.3...v4.5.4)

---
updated-dependencies:
- dependency-name: typescript
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [jest](https://github.com/facebook/jest) from 27.4.4 to 27.4.5.
- [Release notes](https://github.com/facebook/jest/releases)
- [Changelog](https://github.com/facebook/jest/blob/main/CHANGELOG.md)
- [Commits](jestjs/jest@v27.4.4...v27.4.5)

---
updated-dependencies:
- dependency-name: jest
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) from 5.6.0 to 5.7.0.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.7.0/packages/eslint-plugin)

---
updated-dependencies:
- dependency-name: "@typescript-eslint/eslint-plugin"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Makes things more coherent
Bumps [ts-jest](https://github.com/kulshekhar/ts-jest) from 27.1.1 to 27.1.2.
- [Release notes](https://github.com/kulshekhar/ts-jest/releases)
- [Changelog](https://github.com/kulshekhar/ts-jest/blob/main/CHANGELOG.md)
- [Commits](kulshekhar/ts-jest@v27.1.1...v27.1.2)

---
updated-dependencies:
- dependency-name: ts-jest
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Using eslint-plugin-sonarjs. The plugin isn't working well enough to include it (especially the detection of unnecessary switch branches doesn't work, which is the feature I was looking for).
They will now be closed when we handle the properly closed tag.
Bumps [eslint](https://github.com/eslint/eslint) from 8.4.1 to 8.5.0.
- [Release notes](https://github.com/eslint/eslint/releases)
- [Changelog](https://github.com/eslint/eslint/blob/main/CHANGELOG.md)
- [Commits](eslint/eslint@v8.4.1...v8.5.0)

---
updated-dependencies:
- dependency-name: eslint
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
)

Bumps [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser) from 5.7.0 to 5.8.0.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/parser/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.8.0/packages/parser)

---
updated-dependencies:
- dependency-name: "@typescript-eslint/parser"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) from 5.7.0 to 5.8.1.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.8.1/packages/eslint-plugin)

---
updated-dependencies:
- dependency-name: "@typescript-eslint/eslint-plugin"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
)

Bumps [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser) from 5.8.0 to 5.8.1.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/parser/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.8.1/packages/parser)

---
updated-dependencies:
- dependency-name: "@typescript-eslint/parser"
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@43081j
Copy link
Member

43081j commented Dec 28, 2021

this works for what you have because T extends TypeMap implies the default, T extends TypeMap<{[adapterKey]?: 'Document'}, ...>.

you're branding the types as a way to turn something unknown, known, i understand.

but the typemap type itself becomes meaningless and unusable:

type MyTypeMap = TreeAdapterTypeMap<
  {type: 'doc'},
  {type: 'docFrag'},
  {type: 'element'},
  {type: 'comment'},
  {type: 'text'},
  {type: 'template'},
  {type: 'docType'}
>;

declare const adapter: Adapter<MyTypeMap>;

this will error, of course, because you said extends TreeAdapterTypeMap<{[adapterKey]?: 'Document'}, ...>.

so all tree adapters now have to implement your fake adapterKey brand in order to work in the type system.

type maps unfortunately can't be made to work. i've tried many different potential solutions, and the simplest seems to be to dumb down the tree adapter interface for consumption while keeping the concrete tree adapter types rich. happy to push a branch with that once i get it building

@fb55
Copy link
Author

fb55 commented Dec 28, 2021

so all tree adapters now have to implement your fake adapterKey brand in order to work in the type system.

They don't — have a look at the existing tree adapters. Because the key is optional, any object can be passed.

@fb55
Copy link
Author

fb55 commented Dec 28, 2021

The downside is that a call like appendChild(parent, {foo: 'bar'}) is valid. BUT all properly created nodes will be blocked, as their [adapterKey] property doesn't match.

@43081j
Copy link
Member

43081j commented Dec 28, 2021

the code above does not build.

T extends TypeMap is actually constraining to the default types. this means we can't ever use any other type map, only ever those which implement the default.

interface Adapter<T extends TypeMap> {

// is the same as

interface Adapter<T extends TypeMap<
  {[adapterKey]?: 'Document'},
  ...
>> {

// this means we can only ever use type maps which ARE the default, i.e. with an adapterKey

declare const a: Adapter<TypeMap<
  {type: 'doc'},
  ...
>>; // THIS DOES NOT BUILD!

// so we MUST use the default

declare const a: Adapter<TypeMap<
  {[adapterKey]?: 'Document'; type: 'doc'},
  {[adapterKey]?: 'DocumentFragment'; type: 'docFrag'},
  {[adapterKey]?: 'Element'; type: 'element'},
  {[adapterKey]?: 'CommentNode'; type: 'comment'},
  {[adapterKey]?: 'TextNode'; type: 'text'},
  {[adapterKey]?: 'Template'; type: 'template'},
  {[adapterKey]?: 'DocumentType'; type: 'docType'}
>>; // BUILDS!

so yes, yes we do need to specify the adapter key every time we want a custom type map.

@43081j
Copy link
Member

43081j commented Dec 28, 2021

if you give me some time to get my branch building, i can show you my suggestion. basically, dumb down the input types and keep the concrete types strong. it means we lose some type information on the way in, but at least it would be simple and accurate.

@43081j
Copy link
Member

43081j commented Dec 29, 2021

i opened #43 to show what it'd look like if we just typed the generics normally.

in this solution: the typemap becomes irrelevant, as we're actually constraining on its default parameters rather than the type itself.

in the solution in #43: it is overly verbose because we need to specify all type params, every time (however that is what we should be doing really, all of this is us trying to avoid writing them all out and find a shortcut in reality)

there's also a solution somewhere i have yet to figure out: consume non-generic tree adapters but strongly type each individual tree adapter. no clue how this'd work but it sounds like there's an idea there somewhere.

@fb55 fb55 force-pushed the master branch 2 times, most recently from 68cf1f2 to 069852f Compare January 7, 2022 02:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants