-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
fix(core) add a replacer to handle functions/classes when creating mo… #2457
fix(core) add a replacer to handle functions/classes when creating mo… #2457
Conversation
Pull Request Test Coverage Report for Build 3344
💛 - Coveralls |
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.
@ZenoZen Great PR, well done!
Can you try to run at least the integration/injector
integration tests? That should run without any Docker container running. Use mocha integration/injector/**/*.spec.ts --reporter spec --require ts-node/register --require 'node_modules/reflect-metadata/Reflect.js'
If you get it running, could you also add a new test case into the integration/injector
folder to mimic your case of having a Dynamic Module with providers - so we do not break it again?
@@ -50,4 +50,12 @@ export class ModuleTokenFactory { | |||
const scope = Reflect.getMetadata(SHARED_MODULE_METADATA, metatype); | |||
return scope ? scope : 'global'; | |||
} | |||
|
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.
Can you add a comment about why this replacer function exists in the first place? So future contributors won't get confused and delete this function :)
@BrunnerLivio Ah, will do. I tried to run all integration test suite and failed with some syntax error. Your command does successfully run the injector integration tests. Will make the change tonight after getting off work, thanks for the review :) |
return value.name; | ||
} else { | ||
return value; | ||
} |
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.
💡 tip: You dont need this else
statement.
if (typeof value === 'function') {
return value.name;
}
return value;
I have added a comment :) Amazing contribution, thank you! |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
…dule tokens
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #2379
What is the new behavior?
ModuleTokenFactory now correctly handles metadata with class/function objects in.
Does this PR introduce a breaking change?
Other information