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

Fix encoding management #413

Merged
merged 10 commits into from
Apr 24, 2024
Merged

Fix encoding management #413

merged 10 commits into from
Apr 24, 2024

Conversation

CGNonofr
Copy link
Contributor

@CGNonofr CGNonofr commented Apr 19, 2024

  • Restore the usage of @vscode/iconv-lite-umd/jschardet to detect/handle different encoding (they are lazy-loaded)
  • Remove some mocked file
  • Attempt to move as much code as possible to the leaf packages: it allowed to remove almost 2mo from the main package

Loïc Mangeonjean added 2 commits April 18, 2024 16:37
as it seems they are not used anymore from outside and will be only contained by the sync service override
@CGNonofr CGNonofr requested review from CompuIves and kaisalmen April 19, 2024 17:28
@CGNonofr CGNonofr force-pushed the fix-encoding-management branch from ab3e4d1 to 7563299 Compare April 19, 2024 17:32
Copy link
Collaborator

@kaisalmen kaisalmen left a comment

Choose a reason for hiding this comment

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

Local test look good and potentially smaller bundles are very welcome by end-users.
You introduced a new service and did not document it that's why I request changes.

@CGNonofr
Copy link
Contributor Author

Local test look good and potentially smaller bundles are very welcome by end-users. You introduced a new service and did not document it that's why I request changes.

Since it's an internal service override, I was wondering if it was worth it to document it 🤔

@kaisalmen
Copy link
Collaborator

Since it's an internal service override, I was wondering if it was worth it to document it 🤔

Yes, that makes sense

Copy link
Collaborator

@kaisalmen kaisalmen left a comment

Choose a reason for hiding this comment

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

LGTM

@kaisalmen
Copy link
Collaborator

@CGNonofr Are more changes upcoming or is there another reason you did not merge this, yet?

@CGNonofr
Copy link
Contributor Author

@CGNonofr Are more changes upcoming or is there another reason you did not merge this, yet?

There are some... experiments, but if it leads to anything it'll be in another MR

@CGNonofr CGNonofr merged commit 56598e6 into main Apr 24, 2024
2 checks passed
@CGNonofr CGNonofr deleted the fix-encoding-management branch April 24, 2024 11:57
CGNonofr added a commit that referenced this pull request Apr 25, 2024
Copy link

🎉 This PR is included in version 4.5.0-improve-code-splitting.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@CGNonofr
Copy link
Contributor Author

@CGNonofr Are more changes upcoming or is there another reason you did not merge this, yet?

We can release anytime if you want! I don't have anything else in the pipe though :)

@kaisalmen
Copy link
Collaborator

We can release anytime if you want! I don't have anything else in the pipe though :)

We can combine it with other PR. No pressure from my side. I am busy with something else this week.

Copy link

🎉 This PR is included in version 4.5.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants