Skip to content

Upgrade Markdown-it#179581

Merged
kc13greiner merged 7 commits intoelastic:7.17from
kc13greiner:feature/upgrade-markdown-it
Apr 23, 2024
Merged

Upgrade Markdown-it#179581
kc13greiner merged 7 commits intoelastic:7.17from
kc13greiner:feature/upgrade-markdown-it

Conversation

@kc13greiner
Copy link
Copy Markdown
Contributor

@kc13greiner kc13greiner commented Mar 27, 2024 •
edited
Loading

Summary

Upgrade markdown-it from 12.3.2 to 14.1.0

Changes

markdown-it/markdown-it@12.3.2...14.1.0

@kc13greiner kc13greiner added chore Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting v7.17.20 labels Mar 27, 2024
@kc13greiner kc13greiner self-assigned this Mar 27, 2024
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 27, 2024

Pinging @elastic/kibana-security (Team:Security)

@kc13greiner
Copy link
Copy Markdown
Contributor Author

kc13greiner commented Mar 27, 2024

/ci

@kc13greiner kc13greiner enabled auto-merge (squash) March 27, 2024 22:30
@kc13greiner
Copy link
Copy Markdown
Contributor Author

kc13greiner commented Mar 28, 2024

/ci

azasypkin
azasypkin approved these changes Mar 28, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@azasypkin azasypkin left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (code review only) with green CI, thanks!

Hey @elastic/kibana-visualizations and @elastic/response-ops,

I see you rely on the markdown-it package in your code. In this PR, we're bumping this package up by two majors. We're going to merge as soon as CI is green, assuming existing tests would catch any potential breaking changes.

If you're aware of any code paths that depend on markdown-it and are not covered with tests, please verify that everything is still functioning properly when you have time. Thanks!

UPDATE: Looks like our tests have caught something already!

@kc13greiner kc13greiner disabled auto-merge March 28, 2024 13:41
@kc13greiner
Copy link
Copy Markdown
Contributor Author

kc13greiner commented Mar 28, 2024

I would like to fix forward with this upgrade if it is not prohibitively difficult; our current version is over 2 years old!

Who would be best to contact about these Storybook issues?

@legrego
Copy link
Copy Markdown
Member

legrego commented Mar 28, 2024

Who would be best to contact about these Storybook issues?

@elastic/kibana-operations owns the storybook dependency, but if you have problems with specific storybooks, then we'll have to reach out to the respective teams.

@Ikuni17
Copy link
Copy Markdown
Contributor

Ikuni17 commented Apr 2, 2024

@kc13greiner The Storybook build error looks like it is trying to load the ES module instead of Common JS. We currently do not support ES modules, but the markdown-it 14.0 changelog says it should fallback to Common JS. So, you may need a bit of configuration to do that (I didn't dig into it), or only bump to 13.x version.

@pmuellr pmuellr self-requested a review April 11, 2024 21:05
pmuellr
pmuellr approved these changes Apr 11, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the changelog - nothing that would directly impact ResponseOps email connector. Perhaps some better auto-linkification? Who can say :-)

Tested with some emails with Discover links that rarely linkify - no difference from previous releases - broken links, but full text - so no change.

So, LGTM.

@markov00
Copy link
Copy Markdown
Contributor

markov00 commented Apr 12, 2024

Hi there, from the Visualization team POV, this change is fine if the new version doesn't introduce breaking changes in the spec/preferences/defaults. We already had a pretty large issue when trying to migrate to the EUI markdown component because it introduces and changes way too much the style/code/CSS of the output.
Taking a look at the markdown-it code, the changes don't seem to include anything in this regard (only fixes to wrong behaviours). So it should be good for us

@jbudz
Copy link
Copy Markdown
Contributor

jbudz commented Apr 12, 2024

buildkite test this

jbudz
jbudz reviewed Apr 12, 2024
View reviewed changes
packages/kbn-storybook/src/webpack.config.ts Outdated
},
config
);
// webpackMerge is concatenating the storybookConfig and config
Copy link
Copy Markdown
Contributor

@jbudz jbudz Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm somewhat surprised this was working as is, the merged config doesn't look how I would expect it to. This needs a better fix, but assuming it works I'll leave it for the reviewers judgement on priority.

@kc13greiner
Copy link
Copy Markdown
Contributor Author

kc13greiner commented Apr 15, 2024

@pmuellr @markov00 Would you double check that these latest changes work with your usages of this library?

If they are satisfied, I think we can merge.

@pmuellr
Copy link
Copy Markdown
Contributor

pmuellr commented Apr 15, 2024

latest changes continue to work ...

@kc13greiner kc13greiner requested a review from markov00 April 16, 2024 11:16
@kc13greiner
Copy link
Copy Markdown
Contributor Author

kc13greiner commented Apr 23, 2024

/ci

markov00
markov00 approved these changes Apr 23, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@markov00 markov00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, no visual differences between the two versions

@kc13greiner
Copy link
Copy Markdown
Contributor Author

kc13greiner commented Apr 23, 2024

@jbudz Are you ok with me merging this?

@jbudz
Copy link
Copy Markdown
Contributor

jbudz commented Apr 23, 2024

@elastic/kibana-presentation can you verify the canvas shareable runtime behaves as expected?

@jbudz jbudz requested a review from a team April 23, 2024 18:06
@kc13greiner kc13greiner enabled auto-merge (squash) April 23, 2024 18:15
@kibana-ci
Copy link
Copy Markdown

kibana-ci commented Apr 23, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
kibanaReact 351 298 -53
mapsEms 137 85 -52
visTypeVislib 191 138 -53
total -158

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
kibanaReact 219.0KB 252.7KB +33.7KB
mapsEms 201.2KB 237.6KB +36.4KB
visTypeVislib 379.5KB 413.3KB +33.9KB
total +103.9KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
mapsEms 4.1KB 4.1KB -1.0B
visTypeVislib 19.7KB 19.7KB -3.0B
total -4.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @kc13greiner

@kc13greiner kc13greiner merged commit d2dfbd5 into elastic:7.17 Apr 23, 2024
@kc13greiner kc13greiner deleted the feature/upgrade-markdown-it branch April 23, 2024 19:24
@kc13greiner kc13greiner mentioned this pull request May 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pmuellr pmuellr pmuellr approved these changes

@markov00 markov00 markov00 approved these changes

@azasypkin azasypkin azasypkin approved these changes

@jbudz jbudz jbudz approved these changes

Assignees

@kc13greiner kc13greiner

Labels

backport:skip This PR does not require backporting chore release_note:skip Skip the PR/issue when compiling release notes Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// v7.17.20

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@kc13greiner @elasticmachine @legrego @Ikuni17 @markov00 @jbudz @pmuellr @kibana-ci @azasypkin