-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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(apollo-client): add method interestedChangedKeys to ConfigChangeEvent #3666
Merged
nobodyiam
merged 20 commits into
apolloconfig:master
from
Anilople:feature/interested/key/20210508
Jun 11, 2021
Merged
Changes from 2 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
817c9a7
feat(apollo-client): add method interestedChangedKeys to ConfigChange…
Anilople 2b28a27
docs: add Apache LICENSE-2.0
Anilople 1e40d80
fix(apollo-client): null exception
Anilople 2dda71b
Merge branch 'master' into feature/interested/key/20210508
nobodyiam 67d49bc
Merge branch 'master' into feature/interested/key/20210508
Anilople 835823d
delete getChanges in ConfigChangeEvent
Anilople e6c20a0
docs: update LICENSE
Anilople 66d59ee
feat: use InterestedConfigChangeEvent
Anilople cca7e5b
revert a test back
Anilople 0ce9957
test: interested key config change event
Anilople 428f461
Merge branch 'master' into feature/interested/key/20210508
Anilople a3a5cfa
test: add another key
Anilople c475174
Merge branch 'feature/interested/key/20210508' of https://github.com/…
Anilople 2880a4f
Update CHANGES.md
Anilople 3064aaf
Merge branch 'master' into feature/interested/key/20210508
Anilople e78a706
feat: fireConfigChange return how many listener be fired
Anilople 259cda1
test: expose bug in fireConfigChange
Anilople cd30d53
fix: notify multiple time if exists multiple listener
Anilople 5759735
feat: move InterestedConfigChangeEvent, it cannot be use by other pac…
Anilople 479a5d2
rollback fireConfigChange return type to 'void'
Anilople File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 generate a new ConfigChangeEvent here?
Subclass's ConfigChangeEvent + resolved interestedChangedKeys ==> new ConfigChangeEvent
That will change the default behavior of subclass, but still keep compatible.
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 we create a subclass of
ConfigChangeEvent
, which contains the original config change event and the interested keys so that we don't need to create a new instance?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.
When subclasses of
AbstractConfig
invoke methodcom.ctrip.framework.apollo.internals.AbstractConfig#fireConfigChange
, they don't know about any information ofinterestedKeys
becauseinterestedKeys
are saved inAbstractConfig
.If we create a subclass of
ConfigChangeEvent
, when methodcom.ctrip.framework.apollo.internals.AbstractConfig#fireConfigChange
is invoking, we still need to create a new instance ofsubclass of ConfigChangeEvent
, because the listener need to get this instance?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.
you are right, I was thinking we may use the
composition
pattern to avoid create a lot ofConfigChangeEvent
instances.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.
Change
ConfigChangeEvent
to an abstract class?Or create a new class
AbstractConfigChangeEvent
,then let
ConfigChangeEvent
extends it?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.
My original idea was to create an abstract class or an interface, and then use
composition
to wrap the originalConfigChangeEvent
into InterestedConfigChangeEvent. But the current implementation also looks good to me.