Skip to content

Remove immutable package#3147

Merged
koesie10 merged 1 commit intomainfrom
koesie10/remove-immutable
Dec 15, 2023
Merged

Remove immutable package#3147
koesie10 merged 1 commit intomainfrom
koesie10/remove-immutable

Conversation

@koesie10
Copy link
Copy Markdown
Member

This removes the immutable package since it's only used in 1 file and it's not really necessary in that file. I've rewritten the places where it's used in the simplest way possible (i.e. just replacing I.List with an array and I.Map with a standard Map). This removes approximately 175 KB from the bundled extension.js file.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@koesie10 koesie10 marked this pull request as ready for review December 14, 2023 14:56
@koesie10 koesie10 requested a review from a team as a code owner December 14, 2023 14:56
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

LGTM.

Shame there aren't any unit tests. Have you done any testing to confirm the change?

@koesie10
Copy link
Copy Markdown
Member Author

Shame there aren't any unit tests. Have you done any testing to confirm the change?

I haven't done any manual testing because I don't know how to exactly get this code to trigger and what I should be looking for. However, this is tested by some unit tests: test/unit-tests/log-scanner.test.ts.

@charisk
Copy link
Copy Markdown
Contributor

charisk commented Dec 14, 2023

Shame there aren't any unit tests. Have you done any testing to confirm the change?

I haven't done any manual testing because I don't know how to exactly get this code to trigger and what I should be looking for. However, this is tested by some unit tests: test/unit-tests/log-scanner.test.ts.

Ah I was looking for them in a join-order.test.ts or something like that.

@koesie10 koesie10 merged commit 34bd7c2 into main Dec 15, 2023
@koesie10 koesie10 deleted the koesie10/remove-immutable branch December 15, 2023 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants