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

Update types for Context, ActionHandler, and Cachers Base #626

Merged
merged 9 commits into from
Dec 3, 2019

Conversation

glenn-p-ryan
Copy link
Contributor

@glenn-p-ryan glenn-p-ryan commented Nov 29, 2019

📝 Description

This PR updates types for Context and ActionHandler which, as a result, should solve the problem discussed in PR 600. Additionally, the type defaultKeygen has been added to the Cachers namespace under the Base class. This method type for generating the key seems to be missing.

🎯 Relevant issues

Issue being discussed here

💎 Type of change

  • Bug fix (non-breaking change which fixes an issue)

🚦 How Has This Been Tested?

Overrode types in our current implementation of moleculer and tested for ts errors.

🏁 Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • New and existing unit tests pass locally with my changes

shawnmcknight and others added 7 commits October 17, 2019 15:16
Transit options missing type definition for disableVersionCheck as specified in v0.13 broker documentation
…efinitions

Added Type Definition disableVersionCheck for BrokerTransitOptions
- ActionHandler accepts Context<any, any>
- Context constructed with unknown params
- defaultKeygen type added to Cachers
- CallMiddlewareHandler type should not be included
@glenn-p-ryan glenn-p-ryan changed the title Update types for Context, ActionHandler, and Cachers Update types for Context, ActionHandler, and Cachers Base Nov 29, 2019
- the method has a condition for expecting meta to be null
- this condition warrants the possibility to expect null as meta
@coveralls
Copy link

coveralls commented Nov 29, 2019

Pull Request Test Coverage Report for Build 2157

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.281%

Totals Coverage Status
Change from base Build 2150: 0.0%
Covered Lines: 13160
Relevant Lines: 13488

💛 - Coveralls

@icebob
Copy link
Member

icebob commented Nov 30, 2019

@ipetrovic11 @shawnmcknight please review it.

Copy link
Member

@shawnmcknight shawnmcknight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to ActionHandler and Context are what was discussed in that other PR, so this can be a replacement for that PR.

One thing I question is the type of params in the defaultKeyGen method. I believe params no longer has (if it ever had) a restriction on type. That is, you could pass through a string as the params value in a call. However, this is typed as object | null which only represents the non-primitive types and null.

I'm thinking this should probably be typed as any since I do not believe there is a restriction on what type parameters can be. However, I'm not really sure if it is intentional to allow any type through as parameters -- most of the jsdocs throughout moleculer indicate params to be an object, including for the defaultKeyGen. If it is intentional for params to allow any type, then this should probably be typed as any and eliminate the restrictions and the jsdocs for the defaultKeyGen should be updated to reflect that. If the allowing of any parameter type is not by design, then this type is fine as is.

@icebob Can you provide any additional detail on the intent of the type for params?

@icebob
Copy link
Member

icebob commented Nov 30, 2019

I've checked the source, so in the defaultKeygen method, the params can be null. But if it's not null, it should be an object.So, the d.ts modification is correct, I should update the jsdoc according to it.

Copy link
Member

@shawnmcknight shawnmcknight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glenn-p-ryan Can you go ahead and update the jsdocs for the defaultKeygen to match the TS type?

@glenn-p-ryan
Copy link
Contributor Author

In the defaultKeygen method, does the first condition:
if(params || meta){...}
also warrant the need to update the jsdocs for meta to possibly be null as well?

- params can be null
@shawnmcknight
Copy link
Member

In the defaultKeygen method, does the first condition:
if(params || meta){...}
also warrant the need to update the jsdocs for meta to possibly be null as well?

It appears to me that meta will default to empty object in the Context constructor and will only be mutated if the parent context's meta is not loose null or the calling options are not loose null. That implies to me that there is no way for meta is ever be null by the time it gets to a called action, so that check seems unneeded and there shouldn't be a need to document meta in that fashion.

@icebob Do you agree?

Copy link
Member

@shawnmcknight shawnmcknight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@icebob
Copy link
Member

icebob commented Dec 3, 2019

Yes, meta can't be null, it's initialized as {}. Thanks @glenn-p-ryan

@icebob icebob merged commit 53f3c4d into moleculerjs:next Dec 3, 2019
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.

5 participants