-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Consolidate all language definitions used in monaco within the @kbn/monaco package #208950
Consolidate all language definitions used in monaco within the @kbn/monaco package #208950
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
0ab27a8
to
0ee2273
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
b9fc849
to
f676ece
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
1 similar comment
/ci |
f27a11c
to
982fdca
Compare
982fdca
to
b84742b
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
1 similar comment
This comment was marked as duplicate.
This comment was marked as duplicate.
💔 Build Failed
Failed CI StepsHistory
cc @eokoneyo |
37fcb72
to
3d9deb9
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
1 similar comment
This comment was marked as duplicate.
This comment was marked as duplicate.
/ci |
9569ecd
to
b86742b
Compare
/ci |
8e726f7
to
980b645
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
Does this have to be backported to 9.0? Doesn't seem like this is a bugfix |
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.
Vis changes LGTM
f9f693e
to
e0359c8
Compare
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.
test/tsconfig.json
changes LGTM
@@ -21,8 +21,7 @@ export const getParsedRequestsProvider = (model: monaco.editor.ITextModel | null | |||
}; | |||
|
|||
// Theme id is the same as lang id, as we register only one theme resolver that's color mode aware | |||
export const CONSOLE_THEME_ID = CONSOLE_LANG_ID; |
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 think it is confusing to use CONSOLE_LANG_ID
instead of CONSOLE_THEME_ID
for the following reason: Console has two languages - CONSOLE_LANG_ID
(for the input panel) and CONSOLE_OUTPUT_LANG_ID
(for the output panel) but both input and output editors have the same theme (with id CONSOLE_THEME_ID
). With this new approach, the Output editor (in src/platform/plugins/shared/console/public/application/containers/editor/monaco_editor_output.tsx
) would have CONSOLE_OUTPUT_LANG_ID
language and CONSOLE_LANG_ID
theme which doesn't make sense. I think we should keep the theme const - is there a reason why you wanted to remove 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.
Fair enough, thanks for pointing out the distinction. It wasn't clear just by looking at the code, I made some adjustments in 17918d9; definitions for console and console output now reside within the language file, in addition there's a separate theme constant for both the input and output appearance.
@elasticmachine merge upstream |
1 similar comment
@elasticmachine merge upstream |
aead3fb
to
cc429c0
Compare
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.
Thanks for addressing my feedback @eokoneyo! Latest changes lgtm.
cc429c0
to
b93a8bb
Compare
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Public APIs missing comments
Any counts in public APIs
Async chunks
Page load bundle
Unknown metric groupsAPI count
History
cc @eokoneyo |
Summary
This PR was created in response to #208858, it migrates all existing language definitions within the
@kbn/code-editor
package into the@kbn/monaco
package to provide a separation of concern for logic that doesn't particularly relate to the configurations for the code editor UI.With this change, all supported languages are ingested from the
@kbn/monaco
package where they will be domiciled from henceforth, and in turn fix the issue that was discovered relating to the way the language definitions within@kbn/code-editor
get registered as a side effect.With this change, to add support for a new language, said language should be defined within
@kbn/monaco
, and registered inlanguages/index.ts
.