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

Allow using 'as const' for creating tuples for map.replace #3344

Merged

Conversation

Nokel81
Copy link
Contributor

@Nokel81 Nokel81 commented Mar 21, 2022

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)

This is really helpful after #3287 since that PR made it so that this use case failed to type check.

I added a test to make sure that this doesn't regress.

@changeset-bot
Copy link

changeset-bot bot commented Mar 21, 2022

🦋 Changeset detected

Latest commit: d92353b

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

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

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

@kubk
Copy link
Collaborator

kubk commented Mar 23, 2022

Thank you for the PR. If you explicitly specify the type, this TS error will also disappear:

const dispose = mobx.reaction(
  () => {
    const values: Array<[string, number]> = Array.from(srcValues, (value, index) => [value, index])
    return values;
  },
  (entries) => map.replace(entries)
)

Considering that there is no replace method in ES 6 Map it might be a better alternative to complicate already complex types :) But there is one more issue.

This example compiles successfully:

const srcValues: string[] = [];
const map = new Map<string, number>(
  Array.from(srcValues, (value, index) => [value, index] as const);
);

This one doesn't

const srcValues = mobx.observable.array<string>()
const map = mobx.observable.map<string, number>(
  Array.from(srcValues, (value, index) => [value, index] as const)
)

That is because TS Map constructor has the following definition:

interface MapConstructor {
    new(): Map<any, any>;
    new<K, V>(entries?: readonly (readonly [K, V])[] | null): Map<K, V>;
    readonly prototype: Map<any, any>;
}

And your PR fixes the issue 👍 So LGTM from me to merge this. Let's wait for other opinions.

@kubk kubk requested review from kubk March 23, 2022 07:10
@urugator
Copy link
Collaborator

Is there a similar issue with Set or Array or possibly object api?

@Nokel81
Copy link
Contributor Author

Nokel81 commented Mar 23, 2022

Maybe, but it is less likely to come up since there is less of a need to use as const since the source array isn't required to be nested.

@kubk
Copy link
Collaborator

kubk commented Mar 23, 2022

@urugator this as const TS pattern is quite common when there is a need to convert an array to ES6 Map. So I doubt there is such an issue with Set/Array

@kubk
Copy link
Collaborator

kubk commented Mar 24, 2022

@Nokel81 Could you also add this line to the test to make sure it is also covered?

const srcValues = mobx.observable.array<string>()
const map = mobx.observable.map<string, number>(
  Array.from(srcValues, (value, index) => [value, index] as const)
)

@urugator
Copy link
Collaborator

Can we somehow wrap this up?

@kubk
Copy link
Collaborator

kubk commented Jul 18, 2022

@Nokel81 We're ready to go once you fix #3344 (comment) and #3344 (comment)

Signed-off-by: Sebastian Malton <sebastian@malton.name>
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81 Nokel81 force-pushed the Allow-readonly-tuples-in-Observable-map-functions branch from 4b1303c to d92353b Compare July 18, 2022 17:07
@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 18, 2022

@kubk Sorry for the delay, done

@kubk kubk merged commit b375535 into mobxjs:main Jul 19, 2022
@kubk
Copy link
Collaborator

kubk commented Jul 19, 2022

@Nokel81 Thank you 👍

@github-actions github-actions bot mentioned this pull request Jul 19, 2022
@Nokel81 Nokel81 deleted the Allow-readonly-tuples-in-Observable-map-functions branch September 8, 2022 12:46
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