-
Notifications
You must be signed in to change notification settings - Fork 1
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: implement plugable map combined with event emitter #403
base: master
Are you sure you want to change the base?
Conversation
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.
looks nice!
packages/plugable/src/plugable.ts
Outdated
@@ -0,0 +1,96 @@ | |||
import { type Plugable, type Key, internals, PlugableInternals, Val } from './types'; | |||
|
|||
const proto = { |
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.
maybe inline the functions on the proto instead of proxy them?
also set the Plugable
type.
set(parent, key, 'hello'); | ||
|
||
expect(res[0]).to.equal('hello'); | ||
}); |
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.
missing tests:
- inherited record doesn't effect the origin
- call return value of
on()
to stop listening - listen with multiple listeners (check that removing 1 doesn't change the others)
}); | ||
}); | ||
|
||
describe('Plugable (prototype 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.
what is this test for? seems like you test the API in the previous tests.
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.
This is related all the other tests checking the non prototype api.
at this point I'm not sure which one is preferred WDYT?
* use getThrow * improve set default value perf
* add more tests * expose createKey on the 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.
How about some docs, examples etc? see other packages in the repo for the formatting
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.
please add ts-docs comments to functions you export, this is used to generate the docs
This is very wordy but I didn't get what's it's used for. can you plz explain? |
No description provided.