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

Apply as const to objects? #5

Closed
mcmire opened this issue Jan 7, 2022 · 3 comments · Fixed by #6
Closed

Apply as const to objects? #5

mcmire opened this issue Jan 7, 2022 · 3 comments · Fixed by #6

Comments

@mcmire
Copy link
Contributor

mcmire commented Jan 7, 2022

I mentioned this in a controllers PR, but I believe that in order to provide proper type safety, each object should have as const on it. That is to say:

export const abiERC1155 = [
  ...
] as const;

Why? Say you have a function or method and you want to ensure that it must take an ERC-1155 ABI. You might think you can say:

import { abiERC1155 } from "@metamask/metamask-eth-abis";

function foo(abi: typeof abiERC1155) {
  // ...
}

But this won't do what you think it will, because the type of the ABI object doesn't verify that you've passed an ERC-1155 ABI exactly, just something that matches the shape.

Let's simplify this:

const value = {
  foo: "bar",
  baz: "qux",
};

The inferred type of this is:

{
  foo: string;
  baz: string;
}

That means that if you have a function foo(x: typeof value), this function will allow foo({ foo: "xxxxx", bar: "yyyyyy" }). Probably not what you want.

But if you add as const you can harden this. The inferred type of:

const value = {
  foo: "bar",
  baz: "qux",
} as const;

is:

{
  foo: "bar";
  baz: "qux";
}

So now foo({ foo: "xxxxx", bar: "yyyyyy" }) won't work anymore, it must be foo({ foo: "bar", baz: "qux" }). The same thing could be done for the ABI objects.

Alternatively, if we don't care about hardening the ABI like this then it might be good to make an ABI type.

@mcmire
Copy link
Contributor Author

mcmire commented Jan 7, 2022

@gantunesr What do you think about this?

@gantunesr
Copy link
Member

Excellent! Totally agree with you @mcmire, it's an easy change that can help avoid possible bugs in the future.

@mcmire
Copy link
Contributor Author

mcmire commented Jan 7, 2022

I have a PR above to address this now :)

@mcmire mcmire closed this as completed in #6 Jan 7, 2022
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 a pull request may close this issue.

2 participants