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

TPromsterOptions not allowing for default options #378

Closed
roikoren755 opened this issue Jun 9, 2020 · 2 comments · Fixed by #379
Closed

TPromsterOptions not allowing for default options #378

roikoren755 opened this issue Jun 9, 2020 · 2 comments · Fixed by #379

Comments

@roikoren755
Copy link
Contributor

Describe the bug
I'm trying to create a middleware for an express app, using @promster/express' createMiddleware function. Before the package moved to TS, the following worked

createMiddleware({ options: { metricTypes: ['countOfGcs', 'durationOfGc', 'reclaimedInGc'] } });

and gave me the metrics I wanted. Now, however, after the move, I get an error from TS stating that buckets and percentiles are missing.

To Reproduce
Steps to reproduce the behavior:

Using TS 3.9, the following shows the error

import { createMiddleware } from '@promster/express';

createMiddleware({ options: { metricTypes: ['countOfGcs', 'durationOfGc', 'reclaimedInGc'] } });

Expected behavior
No TypeScript errors should occur

Additional context
Any place I found in the source code that uses either of these values has a fallback default value, so shouldn't these two be optional?

Right now, TPromsterOptions is defined with

    buckets: [number];
    percentiles: [number];
}

From what I saw, these can be optional in the interface. I can open a PR with this small change, if that would be deemed appropriate, and would help

@tdeekens
Copy link
Owner

tdeekens commented Jun 9, 2020

Thanks for opening such a eloborate issue!

I think what you're suggesting sounds right. Can you open a PR to fix it then we can take the discussion there.

@roikoren755
Copy link
Contributor Author

Opened a PR at #379

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