Removed the text-encondig code line #227

gedu · 2024-12-20T17:16:23Z

Details

Adds a custom transformer to support React Native versions below 0.75, which require the TextEncoder polyfill. The transformer modifies the src/index.js file of the library to include the necessary polyfill.

What this fixes

This PR addresses an issue where the text-encoding library was being included unnecessarily in the bundle for React Native versions 0.75 and above. By conditionally adding the polyfill only when needed, this change reduces the bundle size and improves performance.

Checklist

I have described the bug/issue
I have provided reproduction in Example App
I have tested that solution works on Example App on all platforms:
- Android
- iOS
- Web

Screenshots/Videos

github-actions · 2024-12-20T17:16:36Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

gedu · 2024-12-20T17:16:58Z

I have read the CLA Document and I hereby sign the CLA

szymonrybczak · 2024-12-20T19:58:55Z

README.md

+Make sure your project has a `metro.config.js` file. If not, create one at the root of your project.
+
+```js
+const { getDefaultConfig } = require("expo/metro-config");


here there's usage of expo/metro-config inside next code snippet there's @react-native/metro-config which can be confusing, let's say explicitly that the first one is for Expo projects and second is for Bare React Native

szymonrybczak · 2024-12-20T20:04:08Z

README.md

+  return config;
+})();
+```
+Merge the contents from your project's metro.config.js file with this config (create the file if it does not exist already).


Suggested change

Merge the contents from your project's metro.config.js file with this config (create the file if it does not exist already).

Merge the contents from your project's metro.config.js file with this config.

For Bare React Native it's inside a default template :)

szymonrybczak · 2024-12-20T20:05:01Z

README.md

+
+const { assetExts, sourceExts } = defaultConfig.resolver;
+


Suggested change

const { assetExts, sourceExts } = defaultConfig.resolver;

const { assetExts, sourceExts } = defaultConfig.resolver;

useless?

szymonrybczak

LGTM, left few nits comments

szymonrybczak · 2024-12-20T20:07:35Z

textEncodingTransformation.js

+
+        // React Native versions below 0.75 do not include a global TextEncoder implementation.
+        // To ensure compatibility with these older versions, we add a polyfill using the 'text-encoding' library.
+        if (major === '0' && minor < '75') {


what if React Native will be 1.0? 🙈 maybe we can leverage semver package here since we're in Node.js runtime

Suggested change

if (major === '0' && minor < '75') {

if (semver.lt(version, '0.75.0')) {

szymonrybczak · 2024-12-20T20:07:56Z

textEncodingTransformation.js

+
+module.exports = {
+    transform: createTransformer(upstreamTransformer),
+};


nit: missing new line

AbdulrhmnGhanem · 2024-12-21T13:24:19Z

Wouldn't telling the users to install text-encoding and adding to the globals if they are on RN < 0.75 be simpler? I don't see the added value of the custom transformation, am I missing something?
If I am a first time user, IMHO I would look for something with less overhead.

gedu · 2024-12-23T13:55:18Z

@AbdulrhmnGhanem Thanks for the feedback! I agree that installing a library is simpler, and in mine, users need to tweak their Metro config if they're on RN < 0.75, meaning that it requires a bit of knowledge of it, and everything happens at build time.
This avoids runtime checks and the potential issues with useEffect, like crashes if a component runs before the polyfill is loaded.
@mountiny Happy to hear your thoughts!

AbdulrhmnGhanem · 2024-12-23T14:23:35Z

Thanks for taking the time to respond!

My initial PR is flawed too. I don't think we need an effect if we are going to say it explicitly in the docs:

If you are on RN < 0.75 install the text-encoding package and add the following two lines to import the package

import QRCode from 'react-native-qrcode-svg'

global.TextEncoder = require("text-encoding").TextEncoder;

mountiny · 2024-12-23T14:41:03Z

@gedu I am not sure if I understand the build considerations in our case, we are on 0.75 and we will switch to 0.76. We can note this in the Readme, but we are not sure if we need to worry about it from the App perspective. Am I missing something?

gedu · 2024-12-23T15:06:12Z

On Exfy all should be ok

parasharrajat · 2024-12-23T16:12:59Z

package.json

Please update lock file as well.

Same for the example app.

parasharrajat · 2024-12-23T16:15:54Z

Screenshots

🔲 iOS / native

23.12.2024_23.26.08_REC.mp4

🔲 MacOS / Chrome

23.12.2024_21.41.46_REC.mp4

🔲 Android / native

23.12.2024_21.54.39_REC.mp4

parasharrajat · 2024-12-23T17:57:33Z

@gedu There are small requests but the changes looks good to me.

gedu · 2024-12-23T18:14:13Z

@parasharrajat I will review the npm version, Probably on a follow-up PR we can increase the RN version on the Example app.

parasharrajat · 2024-12-23T18:57:40Z

@gedu Ok, No problem. But we can atleast fix the lock files as they do not match the package.json files. I see changes on npm install and yarn install for both the main pkg and example app.

gedu · 2024-12-24T09:16:57Z

@parasharrajat I Updated the package-lock.json from root, on the Example app, what should I update? I didn't change dependencies there.

gedu · 2024-12-24T11:00:54Z

On Exfy is working fine for 0.75

qrcode-svg-exfy.mp4

parasharrajat · 2024-12-24T11:07:21Z

@gedu Looks like svg version is mismatched on yarn.lock file on Example app. But this is a no blocker for me.

parasharrajat

🎀 👀 🎀 C+ reviewed

parasharrajat · 2024-12-24T11:08:37Z

All yours @mountiny

Removed the text-encondig code line

222953b

CLABotify added a commit to Expensify/CLA that referenced this pull request Dec 20, 2024

@gedu has signed the CLA in Expensify/react-native-qrcode-svg#227

9527f5d

szymonrybczak reviewed Dec 20, 2024

View reviewed changes

gedu mentioned this pull request Dec 23, 2024

[$250] Update react-native-qrcode-svg to not use text-decoding Expensify/App#54096

Open

mountiny self-requested a review December 23, 2024 13:11

mountiny assigned gedu Dec 23, 2024

Fixed comments

1f65e6d

mountiny requested a review from thesahindia December 23, 2024 14:42

mountiny requested review from parasharrajat and removed request for thesahindia December 23, 2024 15:14

parasharrajat suggested changes Dec 23, 2024

View reviewed changes

updated lock.json

0bb23d9

parasharrajat approved these changes Dec 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed the text-encondig code line #227

Removed the text-encondig code line #227

gedu commented Dec 20, 2024

github-actions bot commented Dec 20, 2024 •

edited

Loading

gedu commented Dec 20, 2024

szymonrybczak Dec 20, 2024 •

edited

Loading

szymonrybczak Dec 20, 2024

szymonrybczak Dec 20, 2024

szymonrybczak left a comment

szymonrybczak Dec 20, 2024

szymonrybczak Dec 20, 2024

AbdulrhmnGhanem commented Dec 21, 2024 •

edited

Loading

gedu commented Dec 23, 2024

AbdulrhmnGhanem commented Dec 23, 2024

mountiny commented Dec 23, 2024

gedu commented Dec 23, 2024

parasharrajat Dec 23, 2024

parasharrajat Dec 23, 2024

parasharrajat commented Dec 23, 2024 •

edited

Loading

parasharrajat commented Dec 23, 2024

gedu commented Dec 23, 2024

parasharrajat commented Dec 23, 2024

gedu commented Dec 24, 2024

gedu commented Dec 24, 2024

parasharrajat commented Dec 24, 2024 •

edited

Loading

parasharrajat left a comment

parasharrajat commented Dec 24, 2024

	Merge the contents from your project's metro.config.js file with this config (create the file if it does not exist already).
	Merge the contents from your project's metro.config.js file with this config.


	const { assetExts, sourceExts } = defaultConfig.resolver;
	const { assetExts, sourceExts } = defaultConfig.resolver;

	if (major === '0' && minor < '75') {
	if (semver.lt(version, '0.75.0')) {

Removed the text-encondig code line #227

Are you sure you want to change the base?

Removed the text-encondig code line #227

Conversation

gedu commented Dec 20, 2024

Details

What this fixes

Checklist

Screenshots/Videos

github-actions bot commented Dec 20, 2024 • edited Loading

gedu commented Dec 20, 2024

szymonrybczak Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

szymonrybczak Dec 20, 2024

Choose a reason for hiding this comment

szymonrybczak Dec 20, 2024

Choose a reason for hiding this comment

szymonrybczak left a comment

Choose a reason for hiding this comment

szymonrybczak Dec 20, 2024

Choose a reason for hiding this comment

szymonrybczak Dec 20, 2024

Choose a reason for hiding this comment

AbdulrhmnGhanem commented Dec 21, 2024 • edited Loading

gedu commented Dec 23, 2024

AbdulrhmnGhanem commented Dec 23, 2024

mountiny commented Dec 23, 2024

gedu commented Dec 23, 2024

parasharrajat Dec 23, 2024

Choose a reason for hiding this comment

parasharrajat Dec 23, 2024

Choose a reason for hiding this comment

parasharrajat commented Dec 23, 2024 • edited Loading

Screenshots

🔲 iOS / native

🔲 MacOS / Chrome

🔲 Android / native

parasharrajat commented Dec 23, 2024

gedu commented Dec 23, 2024

parasharrajat commented Dec 23, 2024

gedu commented Dec 24, 2024

gedu commented Dec 24, 2024

parasharrajat commented Dec 24, 2024 • edited Loading

parasharrajat left a comment

Choose a reason for hiding this comment

parasharrajat commented Dec 24, 2024

github-actions bot commented Dec 20, 2024 •

edited

Loading

szymonrybczak Dec 20, 2024 •

edited

Loading

AbdulrhmnGhanem commented Dec 21, 2024 •

edited

Loading

parasharrajat commented Dec 23, 2024 •

edited

Loading

parasharrajat commented Dec 24, 2024 •

edited

Loading