-
Notifications
You must be signed in to change notification settings - Fork 979
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
[CCI] Add bluebird replaces for plugins/vis_type_tagcloud #4028
[CCI] Add bluebird replaces for plugins/vis_type_tagcloud #4028
Conversation
Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
Codecov Report
@@ Coverage Diff @@
## main #4028 +/- ##
=======================================
Coverage 66.43% 66.43%
=======================================
Files 3229 3229
Lines 62069 62069
Branches 9599 9599
=======================================
Hits 41234 41234
Misses 18531 18531
Partials 2304 2304
Flags with carried forward coverage won't be shown. Click here to find out more. |
Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
649d8dc
to
ab39915
Compare
@Nicksqain Can you add the missing changelog entry to the PR? |
Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
…qain/OpenSearch-Dashboards into bluebird-vis_type_tagcloud
Yes, of course. |
I would say |
Yes, thank you. I did, I added it to refactoring. |
I would suggest change log for a PR is a good strategy as long as single PR solves one problem at a time or same problem within the scope of logical separated component. If your PR's are spread across to fix single issue of concern which is in this case I think, it's reasonable to include a changelog in one of the PR's as opposed to changelog in each PR as the primary intention of having changelog here is for the release tracker. |
import { TagCloud } from './tag_cloud'; | ||
import { setHTMLElementOffset, setSVGElementGetBBox } from '../../../../test_utils/public'; | ||
|
||
function delay(ms) { | ||
return new Promise((resolve) => setTimeout(resolve, ms)); |
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.
Are there any benefit in using setImmediate()
over setTimeout()
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.
I don't think there will be any difference in performance in this case.
setTimeout()
has the same priority as delay from bluebird
.
setImmediate()
can be executed earlier, which can cause errors
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.
As @AMoo-Miki suggested in ur other PR, this delay
is used many times. We should move to a single place to avoid the redundance.
await new Promise((resolve) => { | ||
tagCloud.once('renderComplete', () => { | ||
resolve(); | ||
}); | ||
}); |
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.
Since this piece is used so many times, I would recommend you pull it out into a function of its own; something like:
const waitForRenderCompletion = async (tagCloud) =>
new Promise((resolve) => {
tagCloud.once('renderComplete', resolve);
});
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.
+1 to both put this func out and use
tagCloud.once('renderComplete', resolve));
cuz (() => { resolve(); })
doesn't add anything in terms of functionality; it just calls resolve with no arguments.
import { TagCloud } from './tag_cloud'; | ||
import { setHTMLElementOffset, setSVGElementGetBBox } from '../../../../test_utils/public'; | ||
|
||
function delay(ms) { | ||
return new Promise((resolve) => setTimeout(resolve, ms)); |
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.
As @AMoo-Miki suggested in ur other PR, this delay
is used many times. We should move to a single place to avoid the redundance.
await new Promise((resolve) => { | ||
tagCloud.once('renderComplete', () => { | ||
resolve(); | ||
}); | ||
}); |
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.
+1 to both put this func out and use
tagCloud.once('renderComplete', resolve));
cuz (() => { resolve(); })
doesn't add anything in terms of functionality; it just calls resolve with no arguments.
Convert to draft for now, can be marked as ready again when the requested changes are resolved. |
@Nicksqain Due to an extended period without updates, we're going to close this issue for now. We appreciate your contribution and understand that priorities and availability can change. Please feel free to reopen this issue when you have the opportunity to revisit it. Thank you for your understanding. |
Description
Issues Resolved
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr