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.

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

Remove costly refactorings #934

Merged
merged 7 commits into from
Oct 28, 2023
Merged

Remove costly refactorings #934

merged 7 commits into from
Oct 28, 2023

Conversation

nicoespeon
Copy link
Owner

This PR removes 2 refactorings:

  1. Destructure Object
  2. Extract useCallback()

Destructure Object

This one was not working well already (#416). It depended on a POC to resolve the types at runtime, which required the inclusion of the whole "typescript" library in the bundle.

Furthermore, I realized that TS 3.4 introduced a similar refactoring to VS Code: https://devblogs.microsoft.com/typescript/announcing-typescript-3-4/#convert-parameters-to-destructured-object

Extract useCallback()

This refactoring was specific to React. In short, it was applying the auto-fix of the eslint-plugin-react-hooks package.

While this was useful, it's an edge-case that only applies to React codebases. It was not really causing performance issues (although any extra refactoring has a cost), but it required to include many packages in the bundle, including the "typescript" library (used by ESLint and the plugin that was also included).

Impact

The size of the extension bundle went down from ~9MB to ~1.5MB:

Before After
before after

And for the browser bundle (the one that runs on VS Code Web), it went down from ~5.5MB to ~1.6MB:

Before After
before-browser after-browser

@nicoespeon nicoespeon mentioned this pull request Oct 28, 2023
4 tasks
@nicoespeon nicoespeon merged commit e6bc6f5 into main Oct 28, 2023
@nicoespeon nicoespeon deleted the remove-costly-refactorings branch October 28, 2023 18:10
@nicoespeon
Copy link
Owner Author

And somehow it seems that Travis stopped working… We'll see, but eventually, we will move to GitHub Actions (which we already use for deployments), so that would be one less moving part.

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.

None yet

1 participant