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.
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
[Feature] Datasource selector of multiple datasources #5167
[Feature] Datasource selector of multiple datasources #5167
Changes from 57 commits
3758626
3f6b467
634c074
160c73e
12ab226
c71e0ea
ad55a8a
5ce2363
0042959
1b8a746
23cf242
23aba4a
249f139
116d1c1
2b8f7ff
c8a29b7
b31e16f
4f1dc68
6e8c2d1
e873130
b54dedd
94de201
53794f0
50ace85
8cd2fe3
dd4b542
467b177
025cff7
20487e0
4866312
5532d38
afc8a9f
e2e0f19
7e54205
78b8b83
3b43cae
0758f4e
422da0b
f01ca6c
b967036
3be325f
dcc8b2f
759f3fa
46fe28e
8da7f55
55a1f3a
2227733
64872bd
00491eb
ccbb92b
09f7002
023507b
811347c
e8f219b
3642ef5
a27d7e0
f188b5f
d56bc1d
ff8f3ef
0a1bfd5
21af094
24e6775
fe8cfeb
81ff866
30fc3d6
5ab664b
bea8e75
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
For harmony with other parts of the code, getters should be employed.
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 does "getters should be employed" mean? inferred return type?
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 we type these
any
s correctly?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.
For a singleton, all you should need is
... outside the class, and you don't need to export your factory.
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.
but it will always be initialized and take memeory. current way it's lazy init
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.
discussed offline and you mentioned the code will run at the time it is imported, so not always initialized in the beginning. i believe that would be in this file
OpenSearch-Dashboards/src/plugins/data/public/plugin.ts
Line 220 in 3642ef5
But when i tested locally, exporting here
OpenSearch-Dashboards/src/plugins/data/public/data_sources/datasource/index.ts
Line 17 in a27d7e0
is enough to trigger
const instance = new DataSourceFactory();
. i think the instance is created whenever data plugin is imported, so i still think it's better to initialize at.getInstance()
call than initialize when importing the file. so that if something changed that line to not call.getInstance()
, no extra instances are createdThere 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.
Lets avoid
any
, cant we use a generic here?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 name is too generic and will result in collision.
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 name is too generic and will result in collision.
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 name is too generic and will result in collision.