-
Notifications
You must be signed in to change notification settings - Fork 394
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
feat(wire-service): allow service to add adapter #69
Conversation
Benchmark comparisonBase commit:
|
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.
let's wait for the formal proposal about wire adapters before we merge this.
@diervo Any idea why the benchmark shows substantial improvement? |
@pmdartus very odd, let's merge the benchmark PR and investigate further |
changes in |
@@ -42,4 +42,10 @@ const wireService = { | |||
export default function registerWireService(register, ...adapterGenerators) { |
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.
don't use rest argument in platform code, because it compiles to this:
function registerWireService(register) {
for (var _len = arguments.length, adapterGenerators = Array(_len > 1 ? _len - 1 : 0), _key = 1; _key < _len; _key++) {
adapterGenerators[_key - 1] = arguments[_key];
}
@diervo @rsalvador can we add this to the linting rules?
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.
eslint has the opposite rule (prefer-spread), but we could add a custom rule.
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.
@caridy that will be a breaking change (not backward compatible) for registerWireService
call, I'd prefer to do that in a different PR since it doesn't relate to this one. @kevinv11n fyi
2351968
to
381db1c
Compare
Benchmark comparisonBase commit:
|
registerWireAdapter(adapter); | ||
}, | ||
unregister: (adapterName) => { | ||
return unregisterWireAdapter(adapterName); |
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.
@yungcheng @kevinv11n are we concerned about exposing this API ?
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.
Yes. We need to finalize #74 and then reflect its design in this PR.
changes to wire-decorator.test.js are better addressed in #161 The rest would be addressed by the wire adapter rfc implementation #148 |
Details
We currently create adapter registry at service creation and the registry cannot be augmented afterwards. The change is to allow adapters to be added after wire service is created and registered to the engine.
Does this PR introduce a breaking change?