-
Notifications
You must be signed in to change notification settings - Fork 8.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
「AppArch」Interpreter 👉 New Platform #39329
Merged
Merged
Changes from 41 commits
Commits
Show all changes
45 commits
Select commit
Hold shift + click to select a range
6945097
feat: 🎸 set-up NP data plugin
streamich a3d6d24
refactor: 💡 move interpreter functions registry to NP
streamich d0df7ed
refactor: 💡 move interpreter renderer registry to NP plugin
streamich 411e598
refactor: 💡 move interpreter typesRegistry to NP
streamich 843fadc
refactor: 💡 move interpreter types to NP
streamich b1c81ef
chore: 🤖 import typeRegistry from NP and change TS type folder
streamich cfe9a3d
refactor: 💡 move interpreter expression types to NP
streamich fd3b0af
refactor: 💡 move rest of interpreter common folder to NP plugin
streamich 521fa8a
fix: 🐛 fix TypeScript errors
streamich 276c712
test: 💍 improve typings and test mocks
streamich f0f052c
refactor: 💡 make Interpreter internal registry impl private
streamich c7ad473
test: 💍 inline NP backdoor mock creation in test suites
streamich d923cf1
chore: 🤖 change @kbn/interpreter import paths to try fix errors
streamich ae6d23c
Merge branch 'master' of github.com:elastic/kibana into np-interpreter
streamich 1885809
fix: 🐛 improve core Plugin interfaces
streamich df9ec24
feat: 🎸 add stop() lifecycle to NP data plugins
streamich d0dc76f
refactor: 💡 move interpreter into expressions service data NP
streamich 9681c1d
refactor: 💡 inline Registry @kbn/interpreter class
streamich d6ab499
refactor: 💡 remove dependency on @kbn/interpreter in data pub
streamich 24092bf
refactor: 💡 move interpreter common dir into expressions dir
streamich b744582
fix: 🐛 use TS types in kibana_context
streamich da5669c
feat: 🎸 add types suggested in PR review
streamich 0127e73
feat: 🎸 add semantic interpreter registration functions
streamich d7d5eb6
refactor: 💡 use require for all @kbn/interpreter imports
streamich 4f08a6b
Merge branch 'master' of github.com:elastic/kibana into np-interpreter
streamich 6135155
test: 💍 add Karma test mocks, thx @spalger 🙏
streamich 2abb329
docs: ✏️ update Core docs
streamich f387392
test: 💍 add Sinon stubs for registries
streamich f246f75
chore: 🤖 change import syntax in hopes CI will work
streamich 678aa59
chore: 🤖 set App Architecture as owners of data plugin
streamich 42db35e
docs: ✏️ add README
streamich 1d1a5a5
chore: 🤖 change import in hopes to fix optimizer
streamich f8d56d3
Merge branch 'master' of github.com:elastic/kibana into np-interpreter
streamich e62261c
fix: 🐛 make stop() plugin life-cycle optional
streamich 3b575a1
docs: ✏️ update Core API docs
streamich 2a185bc
Merge branch 'master' of github.com:elastic/kibana into np-interpreter
streamich 82fa012
test: 💍 remove unnecessary Jest mock
streamich e091557
chore: 🤖 don't import from deeply inside a plugin
streamich 6f2c8d0
Merge remote-tracking branch 'upstream/master' into np-interpreter
streamich 8451af5
refactor: 💡 try different interpreter import
streamich 06dbcfb
fix: 🐛 fix Karma mocking
streamich b7fbbee
chore: 🤖 resolve merge conflict with master
streamich 3299129
Merge branch 'master' of github.com:elastic/kibana into np-interpreter
streamich 61033e9
fix: 🐛 fix TypeScript type imports
streamich 62b0115
test: 💍 fix broken test
streamich 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
Validating CODEOWNERS rules …
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
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
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
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
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
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
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
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
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
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
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
Oops, something went wrong.
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.
Why this change?
Record<PluginName, unknown>
is more explicit that the keys will be strings and forces you to construct your own type to deal withunknown
.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.
Exactly, but
Record
does not just say that keys can be strings, but that any string is a valid key. Whereas if you extend{}
you must specify the exact valid properties (not strings).So basically in the screenshot above you can see TypeScript allowing to give "non-existing"
a.foo
to some JavaScript function (which I believe should be an error, if thata
was a plugin contract). But it shows an error if you try the same withb.foo
.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.
Also, there is no
PluginName
alias in./public
.Is
PlaginName
always intended to just be an alias forstring
?If it was somehow possible to narrow it down.
Although there could be 3rd party plugins. And this would need to be auto-generated somehow.
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 true but it's also true that you can't do anything useful with
unknown
. You can't pass it to a function or access any of it's properties without getting an error. The only thing this change does is remove information that the keys of theplugins
argument will bePluginName
. I prefer that this information be retained.We can't do this currently because we can't mix x-pack and OSS code. Maybe we could find a way to generate this, but I don't think it's particularly useful right now.
One way we could make this more useful is with an interface of default types that can be re-opened / merged by plugins.
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.
We could even provide a generic type for generating this:
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 was trying to show in my example above that you can pass it to a function.
Here is another concrete example from Core's source code. If you use
Record
, you can pass anything to a function, becauseRecord<string, ...>
means any property is valid:Whereas, if you restrict the
TPluginSetup
type, you get an error when you want to access property that does not exist:Note, this change still allows the rest of code work as before. Is there anything you cannot achieve with
{}
which you can withRecrod<string, ...>
?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.
Not really. that works due to lax of type signature
alert(message?: any)
. A function with strict arguments will have TS error.I don't see any harm to add your changes to
Plugin
type definitions. Could you update other related files to unifySetup/Start
signatures?1
2
3
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's the whole point, you rely on the fact that the other function will "type check" the property. But
plugins
object itself allows to access any property. If the other function, for example, is a JavaScript function without a type signature, it happily accepts that property.Yes. Do you mean all
TPluginsSetup
andTPluginsStart
types in those files?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.
correct