[proposal] fix: type check issues on encod/decode arguments #139

bananaumai · 2020-10-10T10:55:54Z

About

This PR intends to fix the issue reported on #136 , where I described the workaround for avoiding type check error in TypeScript.

The goal of this PR is making msgpack-javascript available from TypeScript both with/without enabling strictNullCheck compile option.

Since I don't know the actual intention of the original implementer ( @grantila ), the change in this PR might be controversial.

So, though it would be appreciated if you welcome this PR, I highly recommend to ask @grantila 's opinion.

Background

Issue of the current implementation

msgpack-javascript/src/context.ts

Line 3 in 8ba75f8

export type SplitTypes<T, U> = U extends T ? U : Exclude<T, U>;

I think this implementation has some issues.

issue of `SplitTypes` when `strictNullCheck` is disabled:

This short code describes the issue.

As I wrote in the inline comment, given type Split<T, U> = U extends T ? U : Exclude<T, U>, Split<T, undefined> is always evaluated as never unless T is undefined because undefined is subtype of all other types when strictNullCheck is disabled. (see TypeScript: Handbook - Basic Types)

This behavior is the cause of the issue that i reported in #136.

Lets say, if you write the following code in this circumstance:

const extensionCodec = new ExtensionCodec<MyContext>();
// snip...
const context = new MyContext();
const encoded = encode({new MyType(), { extensionCodec, context });

This code wouldn't be able to compiled as I reported.

The reason can be seen by gradually evaluating encode({myType: new MyType<any>()}, { extensionCodec, context });:

encode({myType: new MyType<any>()}, { extensionCodec, context });
// => encode<MyContext>(unknown, { ExtensionCodecType<MyContext>, context });
// => encode<MyContext>(unknown, EncodeOptions<SplitUndefined<MyContext>>)
// => encode(unknown, EncodeOptions<never>)
// { extensionCodec, context } doesn't match EncodeOptions<never>

This code emulates the issue of the actual code.

above issue can be resolved when `strictNullCheck` is enabled, but ...

So, by enabling strictNullCheck, this issue can be solved as I reported in #136.

But, this is not ideal because client code is forced to enable strictNullCheck.

I was wondering if it's possible to achieve the goal (using msgpack-javascript by TypeScript without enabling strictNullCheck and without changing its API).

Then, I found one weird behavior of SplitTypes when the strictNullCheck is enabled.

// When strictNullCheck is enabled
type Split<T, U> = U extends T ? U : Exclude<T, U>;
type SplitUndefined<T> = Split<T, undefined>

let a: SplitUndefined<undefined> = undefined;
let b: SplitUndefined<number> = 1;

// SplitUndefined<number | undefined> is undefined
let c: SplitUndefined<number | undefined> = undefined
let d: SplitUndefined<number | undefined> = 1 // error

(actual code is here)

For me, given the name SplitUndefined, I felt weird if SplitUndefined<number | undefined> is evaluated as undefined. Rather, it seems natural for SplitUndefined<number | undefined> to be evaluated as number, for me.

Besides, Split<T, U> = U extends T ? U : Exclude<T, U> seems logically incorrect.
Given U is not subtype of T, T won't be the Union Type that contains U. Thus, Exclude<T, U> never excludes U.

So, I started to think this is the bug that @grantila ( original implementer of this code ) didn't intend.

It's really long and winding explanation, that was the story I addressed this PR.

About the changes

Essential change in this PR is the following change.

https://github.com/msgpack/msgpack-javascript/pull/139/files#diff-a7af09f7fa8732629ff9579258b5584bR3

The behavior of this Code is like following code:

export type SplitTypes<T, U> = U extends T
    ? Exclude<T, U> extends never ? T : Exclude<T, U>
    : T;

export type SplitUndefined<T> = SplitTypes<T, undefined>;
let a: SplitUndefined<undefined> = undefined;
let b: SplitUndefined<number> = 1;

// SplitUndefined<number | undefined> is undefined
let c: SplitUndefined<number | undefined> = undefined // error
let d: SplitUndefined<number | undefined> = 1

This code behave same regardless of the state of strictNullCheck

The rest of changes are necessary changes so that type system works properly.

codecov-io · 2020-10-10T10:58:20Z

Codecov Report

Merging #139 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #139   +/-   ##
=======================================
  Coverage   98.30%   98.30%           
=======================================
  Files          15       15           
  Lines         943      943           
  Branches      205      205           
=======================================
  Hits          927      927           
  Misses         16       16

Impacted Files	Coverage Δ
src/decode.ts	`100.00% <100.00%> (ø)`
src/decodeAsync.ts	`100.00% <100.00%> (ø)`
src/encode.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f28e981...46d5f32. Read the comment docs.

codecov-commenter · 2020-10-10T10:59:07Z

Codecov Report

Merging #139 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #139   +/-   ##
=======================================
  Coverage   98.30%   98.30%           
=======================================
  Files          15       15           
  Lines         943      943           
  Branches      205      205           
=======================================
  Hits          927      927           
  Misses         16       16

Impacted Files	Coverage Δ
src/decode.ts	`100.00% <100.00%> (ø)`
src/decodeAsync.ts	`100.00% <100.00%> (ø)`
src/encode.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f28e981...46d5f32. Read the comment docs.

codecov-io · 2020-10-10T10:59:45Z

Codecov Report

Merging #139 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #139   +/-   ##
=======================================
  Coverage   98.30%   98.30%           
=======================================
  Files          15       15           
  Lines         943      943           
  Branches      205      205           
=======================================
  Hits          927      927           
  Misses         16       16

Impacted Files	Coverage Δ
src/decode.ts	`100.00% <100.00%> (ø)`
src/decodeAsync.ts	`100.00% <100.00%> (ø)`
src/encode.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f28e981...46d5f32. Read the comment docs.

codecov-commenter · 2020-10-10T11:01:03Z

Codecov Report

Merging #139 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #139   +/-   ##
=======================================
  Coverage   98.30%   98.30%           
=======================================
  Files          15       15           
  Lines         943      943           
  Branches      205      205           
=======================================
  Hits          927      927           
  Misses         16       16

Impacted Files	Coverage Δ
src/decode.ts	`100.00% <100.00%> (ø)`
src/decodeAsync.ts	`100.00% <100.00%> (ø)`
src/encode.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f28e981...46d5f32. Read the comment docs.

codecov-io · 2020-10-10T11:01:14Z

Codecov Report

Merging #139 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #139   +/-   ##
=======================================
  Coverage   98.30%   98.30%           
=======================================
  Files          15       15           
  Lines         943      943           
  Branches      205      205           
=======================================
  Hits          927      927           
  Misses         16       16

Impacted Files	Coverage Δ
src/decode.ts	`100.00% <100.00%> (ø)`
src/decodeAsync.ts	`100.00% <100.00%> (ø)`
src/encode.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f28e981...b623c69. Read the comment docs.

This commit intends to fix the issue reported on msgpack#136

bananaumai · 2020-10-10T12:30:10Z

src/encode.ts

@@ -43,11 +43,11 @@ const defaultEncodeOptions: EncodeOptions = {};
 *
 * The returned buffer is a slice of a larger `ArrayBuffer`, so you have to use its `#byteOffset` and `#byteLength` in order to convert it to another typed arrays including NodeJS `Buffer`.
 */
-export function encode<ContextType>(
+export function encode<ContextType = undefined>(


Unless giving undefined as default type, the following code will cause type mismatch errors:

encode(someValue, { sortKeys: true })

Even though this error can be avoidable by giving explicit type annotation to the code above like encode(someValue, { sortKeys: true } as EncodeOptions), this will be braking change of the API.

bananaumai · 2020-10-10T12:30:34Z

src/decode.ts

@@ -42,11 +42,11 @@ export const defaultDecodeOptions: DecodeOptions = {};
 *
 * This is a synchronous decoding function. See other variants for asynchronous decoding: `decodeAsync()`, `decodeStream()`, `decodeArrayStream()`.
 */
-export function decode<ContextType>(
+export function decode<ContextType = undefined>(


ditto https://github.com/msgpack/msgpack-javascript/pull/139/files#diff-1c15ba03a28a5b41ad552a44b1fdb89fR46

gfx · 2020-10-11T00:56:51Z

Looks good to me. Thanks.

bananaumai force-pushed the fix-type-checking-issue branch from 46d5f32 to 85db7c0 Compare October 10, 2020 11:02

fix: type check issues on encod/decode arguments

b623c69

This commit intends to fix the issue reported on msgpack#136

bananaumai force-pushed the fix-type-checking-issue branch from 85db7c0 to b623c69 Compare October 10, 2020 11:03

bananaumai commented Oct 10, 2020

View reviewed changes

bananaumai changed the title ~~WIP - fix: type check issues on encod/decode arguments~~ [proposal] fix: type check issues on encod/decode arguments Oct 10, 2020

gfx merged commit d60d4e1 into msgpack:master Oct 11, 2020

gfx mentioned this pull request Oct 11, 2020

Revert "[suggestion] chore: add annotation typescript usage in README" #142

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[proposal] fix: type check issues on encod/decode arguments #139

[proposal] fix: type check issues on encod/decode arguments #139

Uh oh!

bananaumai commented Oct 10, 2020 •

edited

Loading

Uh oh!

codecov-io commented Oct 10, 2020

Uh oh!

codecov-commenter commented Oct 10, 2020

Uh oh!

codecov-io commented Oct 10, 2020 •

edited

Loading

Uh oh!

codecov-commenter commented Oct 10, 2020

Uh oh!

codecov-io commented Oct 10, 2020 •

edited

Loading

Uh oh!

bananaumai Oct 10, 2020

Uh oh!

bananaumai Oct 10, 2020

Uh oh!

gfx commented Oct 11, 2020

Uh oh!

Uh oh!

[proposal] fix: type check issues on encod/decode arguments #139

[proposal] fix: type check issues on encod/decode arguments #139

Uh oh!

Conversation

bananaumai commented Oct 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

About

Background

Issue of the current implementation

issue of SplitTypes when strictNullCheck is disabled:

above issue can be resolved when strictNullCheck is enabled, but ...

About the changes

Uh oh!

codecov-io commented Oct 10, 2020

Codecov Report

Uh oh!

codecov-commenter commented Oct 10, 2020

Codecov Report

Uh oh!

codecov-io commented Oct 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codecov-commenter commented Oct 10, 2020

Codecov Report

Uh oh!

codecov-io commented Oct 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bananaumai Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

bananaumai Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

gfx commented Oct 11, 2020

Uh oh!

Uh oh!

bananaumai commented Oct 10, 2020 •

edited

Loading

issue of `SplitTypes` when `strictNullCheck` is disabled:

above issue can be resolved when `strictNullCheck` is enabled, but ...

codecov-io commented Oct 10, 2020 •

edited

Loading

codecov-io commented Oct 10, 2020 •

edited

Loading