Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Upgrade unified to v10.1.2 and remark-parse to 10.0.1 #151276

Closed
wants to merge 1 commit into from
Closed

Upgrade unified to v10.1.2 and remark-parse to 10.0.1 #151276

wants to merge 1 commit into from

Conversation

watson
Copy link
Contributor

@watson watson commented Feb 15, 2023

This is get rid of the dependency to trim v0.0.1 present in remark-parse 8.x. We technically already force-upgrade trim v0.0.1 to v1.0.1 using our resolutions field in package.json, but to keep the content of this field to a minimum we're constantly working on upgrading dependencies that mean we in time can remove resolutions from it.

Review notes

The main intent of this PR was to upgrade remark-parse. However, this upgrade includes some breaking changes and the package is being fed into the unified package, so I thought it was best to upgrade unified as well (though I don't know if not upgrading would have been safe sa well). This does create quite a large diff in yarn.lock, but I think it's always great to have dependencies up-to-date.

Upgrade unified to v10.1.2 and remark-parse to 10.0.1
20f9707 
This is get rid of the dependency to trim v0.0.1 present in remark-parse 8.x
@watson watson added release_note:skip Skip the PR/issue when compiling release notes backport:all-open Backport to all branches that could still receive a release labels Feb 15, 2023
@watson watson requested a review from a team as a code owner February 15, 2023 11:43
@watson watson self-assigned this Feb 15, 2023
@watson watson requested a review from a team as a code owner February 15, 2023 11:43
@patrykkopycinski
Copy link
Contributor

patrykkopycinski commented Feb 15, 2023 •
edited
Loading

Thank you @watson for setting up the PR ❤️
I think however we should first consider bumping the unified version in https://github.com/elastic/eui/blob/main/package.json#L95 so we don't use two different unified versions, as it may cause some issues, especially with the breaking changes introduced in v10
This should also decrease the amount of added deps in yarn.lock

@watson watson added the Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! label Feb 15, 2023
@elasticmachine
Copy link
Contributor

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

@watson
Copy link
Contributor Author

watson commented Feb 15, 2023

I think however we should first consider bumping the unified version in https://github.com/elastic/eui/blob/main/package.json#L95 so we don't use two different unified versions, as it may cause some issues, especially with the breaking changes introduced in v10
This should also decrease the amount of added deps in yarn.lock

@patrykkopycinski very good point! I'll make an issue in EUI to have unified upgraded there first and then make this PR a draft and mark as blocked until EUI has been upgraded.

@watson watson marked this pull request as draft February 15, 2023 11:53
@watson watson mentioned this pull request Feb 15, 2023
Closed
@watson watson added the blocked label Feb 15, 2023
@kibana-ci
Copy link
Collaborator

kibana-ci commented Feb 15, 2023 •
edited
Loading

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #13 / Cases routes Case view navigates to the cases view page for path: /cases/test-id
  • [job] [logs] Jest Tests #13 / Cases routes Case view navigates to the cases view page for path: /cases/test-id/comment-id
  • [job] [logs] Jest Tests #13 / Cases routes Case view user can navigate to the cases view page with read permissions and path: /cases/test-id
  • [job] [logs] Jest Tests #13 / Cases routes Case view user can navigate to the cases view page with read permissions and path: /cases/test-id/comment-id
  • [job] [logs] Jest Integration Tests #2 / checking migration metadata changes on all registered SO types detecting migration related changes in registered types
  • [job] [logs] Jest Integration Tests #2 / migration v2 completes the migration even when a full batch would exceed ES http.max_content_length
  • [job] [logs] Jest Integration Tests #2 / migration v2 fails with a descriptive message when a single document exceeds maxBatchSizeBytes
  • [job] [logs] Jest Integration Tests #2 / migration v2 fails with a descriptive message when maxBatchSizeBytes exceeds ES http.max_content_length
  • [job] [logs] Jest Integration Tests #2 / migration v2 with corrupt saved object documents collects corrupt saved object documents across batches
  • [job] [logs] Jest Tests #8 / Pane renders with display block by default
  • [job] [logs] Jest Tests #8 / Pane renders with display none when visibility is set to false

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cases 601 625 +24
core 414 416 +2
fleet 779 781 +2
kibanaReact 308 310 +2
kibanaUtils 168 170 +2
security 499 501 +2
securitySolution 3723 3749 +26
total +60

Async chunks

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

id before after diff
cases 395.9KB 458.5KB +62.6KB
core 145.5KB 149.1KB +3.6KB
fleet 930.7KB 934.3KB +3.6KB
kibanaReact 208.2KB 211.8KB +3.6KB
kibanaUtils 60.8KB 64.4KB +3.6KB
security 557.3KB 560.9KB +3.6KB
securitySolution 13.8MB 14.1MB ⚠️ +262.7KB
total +343.2KB

History

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

cc @watson

@cnasikas
Copy link
Member

cnasikas commented Feb 15, 2023 •
edited
Loading

In the past, we tried to bump remark-stringify to 9.0.0 but it broke our markdown editor and we had to downgrade to 8.0.3. In the newer version, remark-stringify used a newer version of remark such that the GitHub style tables are removed from remark and placed in a separate plugin called remark-gfm. We put a unit test to catch this bug in the future. I think updating remark would cause the same issue.

cc @elastic/response-ops-cases

@cee-chen
Copy link
Contributor

cee-chen commented Feb 15, 2023 •
edited
Loading

@cnasikas has it right. Upgrading remark and unified to v10+ is a significant impact on the markdown editor and its downstream plugins. I've investigated this on EUI's side side found it to be an extremely high dev left:

elastic/eui#5543 (comment)

If your sole goal is to get rid of [email protected] for security reasons I would strongly recommend instead switching to GitHub's fork of remark-parse-no-trim, which is what EUI did in elastic/eui#6482. It maintains remark-parse as-is from v8.0.3 but without [email protected].

https://www.npmjs.com/package/remark-parse-no-trim

@watson watson mentioned this pull request Feb 16, 2023
Merged
@watson
Copy link
Contributor Author

watson commented Feb 16, 2023

@cee-chen good idea using remark-parse-no-trim. I've opened a PR to do that instead (#151431) and will close this one. It doesn't completely get rid of remark-parse@8 as it still exists a sub-dependency, but at least we now narrowed it down to as few places as possible.

@watson watson closed this Feb 16, 2023
@watson watson deleted the remove-usage-of-old-trim branch February 16, 2023 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrykkopycinski patrykkopycinski Awaiting requested review from patrykkopycinski patrykkopycinski is a code owner automatically assigned from elastic/security-defend-workflows

@gergoabraham gergoabraham Awaiting requested review from gergoabraham gergoabraham is a code owner automatically assigned from elastic/security-defend-workflows

Assignees

@watson watson

Labels
backport:all-open Backport to all branches that could still receive a release blocked release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@watson @patrykkopycinski @elasticmachine @kibana-ci @cnasikas @cee-chen