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

Remove observable.box type inconsistancy #3409

Merged
merged 8 commits into from
May 23, 2022
Merged

Remove observable.box type inconsistancy #3409

merged 8 commits into from
May 23, 2022

Conversation

Nokel81
Copy link
Contributor

@Nokel81 Nokel81 commented May 20, 2022

Unlike observable.map, observable.set, and observable.array where it is possible to create a type correct empty instance without any initial values but like observable.object I think that observable.box should require an initial value.

This is because the following code type checks but will throw an error at runtime:

import { observable } from "mobx";

const myObvs = observable.box<string>();
myObvs.get().includes("hello world"); // this will throw a TypeError

Code change checklist

  • Added/updated unit tests
  • Updated /docs. For new functionality, at least API.md should be updated
  • Verified that there is no significant performance drop (yarn mobx test:performance)

Unlike `observable.map`, `observable.set`, and `observable.array` where it is possible to create a type correct empty instance without any initial values but like `observable.object` I think that `observable.box` should require an initial value.

This is because the following code type checks but will throw an error at runtime:

```ts
import { observable } from "mobx";

const myObvs = observable.box<string>();
myObvs.get().includes("hello world"); // this will throw a TypeError
```
@changeset-bot
Copy link

changeset-bot bot commented May 20, 2022

🦋 Changeset detected

Latest commit: 353fff7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
mobx Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Signed-off-by: Sebastian Malton <sebastian@malton.name>
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@kubk
Copy link
Collaborator

kubk commented May 20, 2022

@urugator For me it seems like a valid change. What are your thoughts on it? I can't think of a usecase where observable.box should be empty by default.

@Nokel81
Copy link
Contributor Author

Nokel81 commented May 20, 2022

What does TypeError: packages/mobx/__tests__/v5/base/observables.js: Emit skipped for language service mean on the test failures?

.changeset/good-toes-wave.md Outdated Show resolved Hide resolved
packages/mobx/__tests__/tsconfig.json Outdated Show resolved Hide resolved
packages/mobx/__tests__/v5/base/observables.ts Outdated Show resolved Hide resolved
packages/mobx/src/api/observable.ts Outdated Show resolved Hide resolved
@kubk
Copy link
Collaborator

kubk commented May 20, 2022

@Nokel81

What does TypeError: packages/mobx/__tests__/v5/base/observables.js: Emit skipped for language service mean on the test failures?

It means that TS is trying to process a file that shouldn't be processed. A possible cause is the new tsconfig introduced in this PR. Can you remove it and check if the error is gone?

@Nokel81
Copy link
Contributor Author

Nokel81 commented May 20, 2022

It means that TS is trying to process a file that shouldn't be processed. A possible cause is the new tsconfig introduced in this PR. Can you remove it and check if the error is gone?

I need the new tsconfig file to make sure that strict mode is enabled for the test file. Will try and fix it.

Signed-off-by: Sebastian Malton <sebastian@malton.name>
@urugator
Copy link
Collaborator

LGTM, thanks both of you.

@urugator
Copy link
Collaborator

CI got stuck again or whats the status here?

@kubk
Copy link
Collaborator

kubk commented May 22, 2022

Hi @Nokel81
I've pulled main branch locally and added your tests to the typescript-tests.ts file. These tests fail even without the new tsconfig.json. How did you run the tests? I tried with 2 commands - yarn test in the root folder and cd packages/mobx && yarn test. Both of them fail. It means that test are already being executed with TS strict mode or at least with strictNullCheck option. That's what you need.

Let me clarify. It's better to avoid adding a new tsconfig.json configuration file because it patches the issue rather than solves it. Ideally we should have only 2 config files - one for tests and another one for build. Across all the packages in the monorepo. Could you verify locally that this new tsconfig.json is not needed?

@kubk
Copy link
Collaborator

kubk commented May 22, 2022

@urugator @Nokel81 I'd be happy to hear your opinion on that:

What if we just forbid to create observable.box() with no initial value? Here are the benefits:

  • It solves the initial issue. A user will have to initialize the box with undefined or null and this value will be propagated to the return type. Example:
const a = observable.box<string | undefined>(undefined); // IObservableValue<string | undefined>
const b = observable.box<string | null>(null); // IObservableValue<string | null>
  • It won't complicate typings. It should be enough to remove the undefined type (?) from the first argument here:
    box: <T = any>(value?: T, options?: CreateObservableOptions) => IObservableValue<T>

The downsides is that it is kinda BC break. But in my opinion it's a bug fix rather than a BC break. This solution also forces a developer to be more explicit, but this explicity also helps TS to infer types correctly.

I've tried to remove the undefined type locally and all the tests passed (including tests added by @Nokel81)

@urugator
Copy link
Collaborator

I think it's not a design issue, but a typesystem problem. If something can exist without a value, it feels somehow natural to make that value optional and on the contrary a bit artifical/weird to actually require box(undefined). But I am not 100% sure about that.
I would prefer if it would keep working without a value, but I am fine if it wouldn't compile with TS. If we could get both it would be the best at this point, so we don't have to deal with BC.

@Nokel81
Copy link
Contributor Author

Nokel81 commented May 22, 2022

@kubk what do you mean that they fail even without the new tsconfig? They pass with the new tsconfig.

As far as I can see CI is green. Without the tsconfig then tsc would complain about the unnecessary // @ts-ignore comments.

@kubk
Copy link
Collaborator

kubk commented May 23, 2022

@Nokel81

Without the tsconfig then tsc would complain about the unnecessary // @ts-ignore comments.

I've added a few changes to the PR to check that. First I added a line that won't compile in TS strict mode: 5491476 , then I removed the new config file: 6e06dc6

Here is related CI job: https://app.circleci.com/pipelines/github/mobxjs/mobx/2025/workflows/b569591f-17a6-4b57-82fb-6905af65506d/jobs/8916

It fails with an error: "Type 'null' is not assignable to type 'string'". It means that tests are already being executed in TS strict mode and we don't have to add a new config file. Lastly I've replaced ts-expect-error with exact type assertions: 0d113ee

Here we can be more specific and check exact types to avoid false positives in the future. CI has passed so I'll merge the PR. Thank you for your help and patience 👍

@kubk kubk requested review from kubk and removed request for kubk May 23, 2022 06:24
packages/mobx/__tests__/tsconfig.json Outdated Show resolved Hide resolved
packages/mobx/__tests__/tsconfig.json Outdated Show resolved Hide resolved
@kubk
Copy link
Collaborator

kubk commented May 23, 2022

@urugator Thanks for you input! Indeed there is no best choice, so let's avoid BC break at least.

@kubk kubk merged commit 8e204c7 into mobxjs:main May 23, 2022
@github-actions github-actions bot mentioned this pull request May 23, 2022
@Nokel81 Nokel81 deleted the patch-2 branch May 24, 2022 19:36
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

Successfully merging this pull request may close these issues.

3 participants