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
Spaces -> Client to NP #54298
Spaces -> Client to NP #54298
Changes from 16 commits
d2883da
8c4516d
cce8d1f
890b685
e0ac89c
6c8c55d
33f04bd
f650303
25fcc60
0a4198f
295773c
1df82cf
5bdf2b9
925ceed
602b11e
03a122f
95d390e
73cc266
7350cfa
dca42cc
35b81ea
e038ff1
4399b59
d6ffb18
d45e63d
3f24902
9ebe560
6fac7e4
2a01095
25008da
cdf2c10
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.
note: can you please explain why we need this? I seems
spaces
is the only x-pack plugin mentioned here and I'm wondering if we should have something similar in x-pack.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.
It was a while ago..I had Karma test failures, so I followed the instructions in the migration guide to "fix" it. I have a lot less in the LP plugin now though, so maybe I'm already past the issue. I'll remove and see what happens. If it fails, I should be able to introduce a mock within the spaces plugin itself.
kibana/src/core/MIGRATION.md
Lines 1564 to 1570 in 5d6dbf0
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.
Update: the LP spaces plugin runs its
legacy.ts
as part of ahack
, so that it runs for all LP plugins. The goal oflegacy.ts
is to call the NPregisterLegacyAPI
, so that the plugin can finish its initialization. In the case of these Karma tests, we need thenpSetup
static import provided bynew_platform.karma.mock
to have thespaces
plugin registered, so thatlegacy.ts
can callregisterLegacyAPI
.So an alternative to placing this here, is to place a guard within
legacy.ts
, to only callregisterLegacyAPI
if thespaces
plugin is actually defined. What do you think about that approach?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.
On the surface it sounds like a better workaround to me, do you have any concerns regarding 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.
And thanks for the explanation!
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.
None at all! Resolved in 3f24902
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.
optional nit: can we use
(): jest.Mocked<ManagementSetup> => ({
to make sure these are up to date when interface changes?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 causes the following to fail:
Because
managementSetup.sections
is of typeSectionsServiceSetup
, rather thanjest.Mocked<SectionsServiceSetup>
.Do you know of a way to tell TS what we really want to do here? I couldn't get it to work, but didn't spend more than 15 minutes on 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.
Hmm, I see. IIRC Platform team used something like
DeeplyMockedKeys
instead ofjest.Mocked
for cases like this, maybe that would work for us too... But feel free to ignore if it takes too much time to fix Management's mocks and types.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.
That worked perfectly, thanks for the tip!
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.