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

Document about EitherN and correct handling of undefined and null #1986

Open
Boshen opened this issue Feb 29, 2024 · 4 comments
Open

Document about EitherN and correct handling of undefined and null #1986

Boshen opened this issue Feb 29, 2024 · 4 comments
Assignees

Comments

@Boshen
Copy link
Contributor

Boshen commented Feb 29, 2024

Recent releases broke Rspack's tests twice, with confusing runtime error messages like "Value is non of these types bool, String without context".

It was suggested to use Either4<Null, Undefined, String, bool>, but quick search of Either4 on napi.rs yields no results.

I suggest we add some usage examples for the following typing cases:

  • foo: null | string
  • foo: null | undefined | string
  • foo?: null | string
  • etc etc for Either3+

I have no idea how they should work with Rust's Option type, is it foo: Option<Undefined> or foo: Option<Either<Null, Undefined>>? 🤔

@Boshen
Copy link
Contributor Author

Boshen commented Feb 29, 2024

Latest 2.16.0 release breaks: web-infra-dev/rspack#5811

This is not debuggable, we cannot figure out which key / value is erroneous

packages/rspack test: FAIL tests/Output.test.js (41 MB heap size)
packages/rspack test:   ● Output › should be cleared the build directory
packages/rspack test:     Value is non of these types `String`, `RegExp`, `Function`,
packages/rspack test:       525 |     const rawOptions = (0, config_1.getRawOptions)(options, this);
packages/rspack test:       526 |     const instanceBinding = require("@rspack/binding");
packages/rspack test:     > 527 |     __classPrivateFieldSet(this, _Compiler_instance, new instanceBinding.Rspack(rawOptions, this.builtinPlugins, {
packages/rspack test:           |                                                      ^
packages/rspack test:       528 |         beforeCompile: __classPrivateFieldGet(this, _Compiler_instances, "m", _Compiler_beforeCompile).bind(this),
packages/rspack test:       529 |         afterCompile: __classPrivateFieldGet(this, _Compiler_instances, "m", _Compiler_afterCompile).bind(this),
packages/rspack test:       530 |         finishMake: __classPrivateFieldGet(this, _Compiler_instances, "m", _Compiler_finishMake).bind(this),
packages/rspack test:       at Compiler._Compiler_getInstance (dist/Compiler.js:527:54)
packages/rspack test:       at Compiler.call [as build] (dist/Compiler.js:393:87)
packages/rspack test:       at build (dist/Compiler.js:358:26)
packages/rspack test:       at Hook.eval [as callAsync] (eval at create (../../node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:6:1)
packages/rspack test:       at Hook.CALL_ASYNC_DELEGATE [as _callAsync] (../../node_modules/tapable/lib/Hook.js:18:14)
packages/rspack test:       at callAsync (dist/Compiler.js:354:32)
packages/rspack test:       at Hook.eval [as callAsync] (eval at create (../../node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:15:1)
packages/rspack test:       at Hook.CALL_ASYNC_DELEGATE [as _callAsync] (../../node_modules/tapable/lib/Hook.js:18:14)
packages/rspack test:       at callAsync (dist/Compiler.js:350:34)
packages/rspack test:       at Compiler.doRun [as run] (dist/Compiler.js:386:13)
packages/rspack test:       at run (tests/Output.test.js:22:5)
packages/rspack test:       at Object.compile (tests/Output.test.js:56:3)

@Brooooooklyn Brooooooklyn self-assigned this Feb 29, 2024
@Brooooooklyn Brooooooklyn added the bug Something isn't working label Feb 29, 2024
@Brooooooklyn
Copy link
Sponsor Member

The Rspack CI was broken by #1971. The cause of confusion lies in Option, not Either. Either strictly differentiates between null and undefined, while the previous implementation of Option was more chaotic, prone to ambiguity. For the usage scenario of Rspack, I suggest explicitly using EitherN instead of Option.

@Brooooooklyn Brooooooklyn removed the bug Something isn't working label Mar 3, 2024
@Boshen
Copy link
Contributor Author

Boshen commented Mar 3, 2024

A documentation around undefined, null and EitherN would be nice 😁

@sup39
Copy link
Contributor

sup39 commented Mar 4, 2024

Hi! I'm the author of #1971. I believe the implementation is compatible with previous versions so I'm not sure why the tests are broken, but I can provide some information about Option and Either.

By default (without specifying use_nullable = true implemented in #1971), an Option field foo: Option<T> in Rust is converted to an optional field foo?: T in TypeScript (instead of a nullable field foo: null | T nor both foo?: null | T since 2.15.1 as a side effect of #1934 that rejects Null value for Option field). In other words, you can think of the Option in Rust as the ? in TypeScript when defining the type of a field.

As for Either and Either3+, they are simply converted to union types. For example, Either3<Null, String, bool> in Rust is converted to null | string | boolean in TypeScript.

Therefore, to make a field both optional and nullable (foo?: null | T), you need to use foo: Option<Either<Null, T>>, where the Either<Null, T> part in Rust corresponds to the null | T part in TypeScript, and the foo: Option<...> part in Rust corresponds to the foo?: ... part in TypeScript.

Here are more examples. To generate the following interface:

interface Foo {
  required2: null | string
  required3: null | undefined | string
  required4: null | undefined | string | boolean
  optional2?: null | string
  optional3?: null | undefined | string
  optional4?: null | undefined | string | boolean
}

you can define the following struct in Rust:

#[napi(object)]
pub struct Foo {
  pub required2: Either<Null, String>,
  pub required3: Either3<Null, Undefined, String>,
  pub required4: Either4<Null, Undefined, String, bool>,
  pub optional2: Option<Either<Null, String>>,
  pub optional3: Option<Either3<Null, Undefined, String>>,
  pub optional4: Option<Either4<Null, Undefined, String, bool>>,
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants