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

ENA-118 Replace text-encoding polyfill with much smaller library, saving 500KB #6899

Merged
merged 4 commits into from
Oct 11, 2024

Conversation

adroitwhiz
Copy link
Contributor

@adroitwhiz adroitwhiz commented Mar 9, 2021

Resolves

Proposed Changes

This PR replaces the text-encoding package with fastestsmallesttextencoderdecoder.

Reason for Changes

The text-encoding package has been deprecated by its maintainer, and also adds 500KB of encoding data for non-UTF-8 text. Because we're only using it to encode UTF-8, that 500KB is entirely unnecessary.

Test Coverage

Must be tested manually.

Here, the TextEncoder API is used to encode assets for the default project. To test this, create a new default project in Legacy Edge, the only browser without native TextEncoder support.

Browser Coverage

Check the OS/browser combinations tested (At least 2)

Mac

  • Chrome
  • Firefox
  • Safari

Windows

  • Chrome
  • Firefox
  • Edge

Linux

  • Firefox

Chromebook

  • Chrome

iPad

  • Safari

Android Tablet

  • Chrome

@adroitwhiz
Copy link
Contributor Author

Test failures seem to be the result of a ChromeDriver version mismatch

@kchadha kchadha removed their assignment Aug 5, 2022
@benjiwheeler
Copy link
Contributor

To be super clear, this way of providing encoding functions is only needed on legacy Edge, right? Does that include basically all known pre-chromium versions of Edge?

@benjiwheeler
Copy link
Contributor

Added "accessibility" label because reducing download size (for both app and website) is meaningful for improving access when data is slow or expensive

@benjiwheeler benjiwheeler changed the title Replace text-encoding polyfill Replace text-encoding polyfill with much smaller library, saving 500KB Aug 16, 2022
@benjiwheeler benjiwheeler changed the title Replace text-encoding polyfill with much smaller library, saving 500KB ENA-118 Replace text-encoding polyfill with much smaller library, saving 500KB Aug 16, 2022
@apple502j
Copy link
Contributor

@benjiwheeler Yep, this library is used by all Legacy (non-Chromium) Edge - and by ONLY the Legacy Edge (at least for supported environments).

@adroitwhiz
Copy link
Contributor Author

Just a reminder that in order to actually remove this huge dependency from the bundle, you'll need to remove it from all the other packages too.

@cwillisf cwillisf assigned cwillisf and unassigned lostvirtue Nov 22, 2022
@cwillisf cwillisf self-requested a review November 22, 2022 15:08
@cwillisf cwillisf merged commit afdec7e into scratchfoundation:develop Oct 11, 2024
1 of 2 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 11, 2024
…xt-encoding

ENA-118 Replace text-encoding polyfill with much smaller library, saving 500KB
Copy link

🎉 This PR is included in version 4.0.37 🎉

The release is available on:

Your semantic-release bot 📦🚀

@adroitwhiz
Copy link
Contributor Author

@cwillisf Since this PR was first opened, I came across https://www.npmjs.com/package/fast-text-encoding, which seems to be far more popular and faster. I can create another PR for that if you want.

You could also consider dropping the polyfill entirely--the only browser which does not support TextEncoder and TextDecoder is Edge Legacy, which has reached EOS and doesn't even seem to be installable anymore.

@cwillisf
Copy link
Contributor

Yeah - I think removing it entirely is the right answer. Honestly, I merged this because it's relatively self-contained, and wanted to send a signal that I'm trying to make at least some small amount of progress 😅

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.

7 participants