Add required default markdown visState #38390
Conversation
The markdownVis expression has the markdown string property configured as required.The current implementation of the markdown vis doesn't have a default value (actually it's undefined) and this cause the error to be thrown when creating a new markdown vis. fix elastic#38127
|
Pinging @elastic/kibana-app-arch |
markov00
added
Feature:ExpressionLanguage
💚 Build Succeeded |
| @@ -202,7 +202,7 @@ export const prepareString = (variable: string, data: string): string => { | |||
| return `${variable}='${escapeString(data)}' `; | |||
| }; | |||
|
|
|||
| export const escapeString = (data: string): string => { | |||
| export const escapeString = (data: string | string): string => { | |||
There was a problem hiding this comment.
is this added | string required, or can it be removed?
There was a problem hiding this comment.
mmm let me remove this, sorry
| @@ -226,7 +226,10 @@ export const buildPipelineVisFunction: BuildPipelineVisFunction = { | |||
| }, | |||
| markdown: visState => { | |||
| const { markdown, fontSize, openLinksInNewTab } = visState.params; | |||
| const escapedMarkdown = escapeString(markdown); | |||
| let escapedMarkdown = ''; | |||
| if (typeof markdown === 'string' || markdown instanceof String) { | |||
There was a problem hiding this comment.
Now that the default value for markdown has been set to an empty string, is there ever a scenario where this wouldn't evaluate to true?
I understand having it as an extra precaution; mostly I'm just curious.
There was a problem hiding this comment.
Nothing prevents future changes to the markdown code to save null, undefined or numbers to the current markdown string object, so one check more is better than one more error.
There can also be the rare case where you changed manually the URL of a markdown and you created a malformed url like this:
http://localhost:5601/xhr/app/kibana#/visualize/create?type=markdown&_g=()&_a=(filters:!(),linked:!f,query:(language:kuery,query:''),uiState:(),vis:(aggs:!(),params:(fontSize:12,markdown:123,openLinksInNewTab:!f),title:'',type:markdown))
where the markdown test is a number instead of a string. Without that check, it will crash the page. In this case the markdown is also not visualized.
Similar if the url is changed with a boolean value.
As written: it's a rare case, but it's at least one more safety check for the escapeString function until we don't have a fully TS implementation of our code.
If you think it's not necessary I can remove that.
There was a problem hiding this comment.
Makes sense! I don't think we need to remove it, mostly I was just curious about the cases you had in mind
💔 Build Failed |
|
jenkins test this please |
💔 Build Failed |
💚 Build Succeeded |
💚 Build Succeeded |
The markdownVis expression has the markdown string property configured as required.The current implementation of the markdown vis doesn't have a default value (actually it's undefined) and this cause the error to be thrown when creating a new markdown vis. fix elastic#38127
The markdownVis expression has the markdown string property configured as required.The current implementation of the markdown vis doesn't have a default value (actually it's undefined) and this cause the error to be thrown when creating a new markdown vis. fix #38127
Summary
The markdownVis expression has the markdown string property configured as required.The current implementation of the markdown vis doesn't have a default value (actually it's undefined) and this cause the error to be thrown when creating a new markdown vis.
I've fixed the issue in the following way:
''fix #38127
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] This was checked for cross-browser compatibility, including a check against IE11[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support[ ] Documentation was added for features that require explanation or tutorials[ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
[ ] This was checked for breaking API changes and was labeled appropriately[ ] This includes a feature addition or change that requires a release note and was labeled appropriately