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

Max safe integer not compatible with graphql type #696

Closed
chladog opened this issue Feb 10, 2021 · 3 comments
Closed

Max safe integer not compatible with graphql type #696

chladog opened this issue Feb 10, 2021 · 3 comments
Assignees
Labels
type: bug 🐛 Something isn't working
Milestone

Comments

@chladog
Copy link
Contributor

chladog commented Feb 10, 2021

Describe the bug
in getSaleableStockLevel method there is this piece of code:

        const inventoryNotTracked =
            variant.trackInventory === GlobalFlag.FALSE ||
            (variant.trackInventory === GlobalFlag.INHERIT && trackInventory === false);
        if (inventoryNotTracked) {
            return Number.MAX_SAFE_INTEGER;
        }

Which returns max JS safe integer, however that is not compatible with GraphlQLs "Int" type. (Int cannot represent non 32-bit signed integer value: 9007199254740991)

Expected behavior
Max number possible for GraphQL Int type.

Environment (please complete the following information):

  • @vendure/core version: v0.18.4
@chladog chladog added the type: bug 🐛 Something isn't working label Feb 10, 2021
@michaelbromley michaelbromley added this to the v1.0.0 milestone Feb 10, 2021
@michaelbromley
Copy link
Member

Is the graphql error occurring in one of the default API operations? Or are you using that method in a custom operation?

@chladog
Copy link
Contributor Author

chladog commented Feb 17, 2021

I used this method for custom "saleable" resolver.

@michaelbromley
Copy link
Member

I considered changing it to:

return 2 ** 31 -1; // max 32-bit signed int, so it is safe for the GraphQL Int type

but I decided against it because I don't want the GraphQL abstraction leaking into a service method like that. Since you control the resolver, it's trivial to add a Math.min() around the result to make it safe for GraphQL.

On a related note, I've just implemented a solution to #442 which will allow you to specify a StockDisplayStrategy which you may find preferable to use in future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug 🐛 Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants