-
Notifications
You must be signed in to change notification settings - Fork 402
fix: createActions should not garble baseclass metadata #1127
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
Conversation
Essence of this is that the target property of the metadata is not changed in-place.
@@ -125,7 +131,7 @@ describe('controller inheritance', () => { | |||
expect(storage.controllers[0].route).to.be.eq('/derivative'); | |||
expect(storage.controllers[1].target).to.be.eq(AutonomousController); | |||
expect(storage.controllers[1].route).to.be.eq('/autonomous'); | |||
expect(storage.actions[0].target).to.be.eq(DerivativeController); | |||
expect(storage.actions[0].target).to.be.eq(AbstractControllerTemplate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional that the abstract controller is registered too? Seems like it wasn't the case before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed it is; the actions in the storage are actually the base data (ActionMetadataArgs
) and are shared amongst all deriving classes. If it were altered (as a previous change introduced), only the first derived class 'gets' the actions; the action from the storage is used to create the ActualMetadata
in createActions()
, and only at that place, the controller.target
is filled in as target
for the action (which is not the action metadata that is subject of this unit test).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, you are right. Seems like we had an incorrect reference mutation, the fix was just obscured a bit by the structure change :).
Thanks for the contribution!
…estack#1127) (#18) Co-authored-by: wamasimba <[email protected]>
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description
A more concise re-implementation of the
createActions()
method. Essence of this is that thetarget
property of the metadata is not changed in-place, which hindered using a base class multiple times.Checklist
Update index.md
)develop
)npm run prettier:check
passesnpm run lint:check
passesFixes
fixes #1124