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

Add README instructions for polyfilling atob #242

Merged
merged 10 commits into from
Nov 14, 2023

Conversation

frederikprijck
Copy link
Member

@frederikprijck frederikprijck commented Oct 30, 2023

Description

With atob not being available in platforms such as react-native, we are adding readme instructions for polyfilling it.

References

#241

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not the default branch

@frederikprijck frederikprijck requested a review from a team as a code owner October 30, 2023 10:20
Copy link
Contributor

@jonkoops jonkoops left a comment

Choose a reason for hiding this comment

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

Looks like you beat me to it @frederikprijck! LGTM!

@Bardiamist
Copy link

Bardiamist commented Oct 30, 2023

Anybody tried this? Because I tried and looks it doesn't works.

@Bardiamist
Copy link

'core-js/actual/atob' also doesn't works

In both cases error is:

Maximum call stack size exceeded (native stack depth), js engine: hermes

What else polyfill can I try?

@jonkoops
Copy link
Contributor

Anybody tried this? Because I tried and looks it doesn't works.

I have not tied running this code, as I don't have a React Native project set up. @hrastnik are you experiencing a similar issue?

@frederikprijck frederikprijck marked this pull request as draft October 30, 2023 12:34
@hrastnik
Copy link

I've tried it and had the same results. At the end I fixed it like this:

Add the package core-js-pure

yarn add core-js-pure

or

npm install core-js-pure

Then add this code somewhere in the app (like index.js).

import atob from "core-js-pure/stable/atob";
import btoa from "core-js-pure/stable/btoa";

global.atob = atob;
global.btoa = btoa;

@frederikprijck
Copy link
Member Author

This seems to be specific to react-native, I assume it does not work well with how core-js detects the global here: https://github.com/zloirock/core-js/blob/master/packages/core-js/internals/global.js#L7-L15

Based on my testing, on React Native, it would use globalThis, while it seems like you want global as per comment above.

I will update this PR .

@frederikprijck frederikprijck marked this pull request as ready for review October 30, 2023 13:08
@jonkoops
Copy link
Contributor

jonkoops commented Oct 30, 2023

Very interesting, this is strange as it looks core-js even has tests specifically for Hermes. It might be worth it to submit an issue to core-js with some steps to reproduce. Since it's a supported runtime, I am sure it will be fixed.

README.md Outdated
```js
import atob from "core-js-pure/stable/atob";

global.atob = atob;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does React Native have support for globalThis? That might be preferential here.

Copy link
Member Author

@frederikprijck frederikprijck Nov 13, 2023

Choose a reason for hiding this comment

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

Based on your investigation, I am considering reverting this and only recommend using the core-js/stable/atob, or whatever polyfill works for the platform of choise. What do you think?

Copy link
Member Author

@frederikprijck frederikprijck Nov 13, 2023

Choose a reason for hiding this comment

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

Even better, I wonder if we should also just recommend base-64, which has been succesfuly used here: https://github.com/techmatters/terraso-mobile-client/pull/567/files

The benefit of base-64 is that it's created by an active and well-known person, so I am confident. It also has 0 dependencies, so even though it hasnt had any release since 2020, I do not think there is any reason to cut releases and it shouldn't mean it's a package to avoid.

jwt-decode was just as stale before we cut the new major, for the same reason. There was no reason to cut any release for it.

Even more so, based on the messages in this PR, I am leaning towards only mentioning base-64 in the readme. What do you think?

Choose a reason for hiding this comment

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

base-64 LGFM

Choose a reason for hiding this comment

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

I think this is a good idea.

Copy link
Member Author

@frederikprijck frederikprijck Nov 13, 2023

Choose a reason for hiding this comment

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

Updated the PR to only mention base-64. LMK what you think. I tried this in your sample app @jonkoops and it seems to work.

Choose a reason for hiding this comment

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

Looks good to me. 👍

Copy link
Member Author

@frederikprijck frederikprijck Nov 13, 2023

Choose a reason for hiding this comment

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

Actually, the repo on base-64 does look kinda stale. We can go with both core-js and base-64 and see if any issues arise and remove the recommendation of base-64 if that's the case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a fine solution @frederikprijck. I am hopeful Hermes might eventually implement this feature, so we might be able to remove this in the future sometime.

@hrastnik
Copy link

@Bardiamist Are you by any chance using MobX, and decoding the token inside a getter/computed?

I did some experiments and noticed everything works as expected when I use a plain string and decode the jwt, but since I use MobX, and I've decoded the token inside a computed function. I'm guessing the atob somehow accesses or writes something on the original string which causes an infinite render loop.

So, after some consideration I've ended up using the base-64 package since it works well and it's a smaller package than core-js-pure.

Here's my final polyFill

import { encode, decode } from "base-64";

global.atob = decode;
global.btoa = encode;

@Bardiamist
Copy link

I don't use MobX.
base-64 works.
Also possible to exctract polyfill from https://github.com/auth0/jwt-decode/blob/14b61ecc2c6e46c2f70f86ccb9a3fd7cf72cfbf4/lib/atob.js

@jonkoops
Copy link
Contributor

I have not been able to reproduce the issues folks in this thread are having in regards to core-js not working in React Native, specifically importing core-js/stable/atob in Hermes.

I set up Android Studio and set up my phone for development, I then followed the instructions for setting up a React Native development environment:

npx create-expo-app AwesomeProject
cd AwesomeProject
npm install core-js jwt-decode
npm run android

I then added the following code to the App.js file:

import "core-js/stable/atob";

const message = atob("SGVsbG8sIHdvcmxk");
console.log(message); // "Hello, world"

I can confirm the message shows up as expected, and does not cause any issues, and that this code breaks as expected when removing the import of core-js. Also expanding this code to use jwt-decode does not seem to make a difference:

import "core-js/stable/atob";
import { jwtDecode } from "jwt-decode"

const token = "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJmb28iOiJiYXIiLCJleHAiOjEzOTMyODY4OTMsImlhdCI6MTM5MzI2ODg5M30.4-iaDojEVl0pJQMjrbM1EzUIfAZgsbK_kgnVyVxFSVo";
const decoded = jwtDecode(token);

console.log(decoded); // { "exp": 1393286893, "foo": "bar", "iat": 1393268893 }

This code runs perfectly fine, I am not seeing issues here. @Bardiamist @hrastnik are you able to reproduce the issue in a minimal example like this? Perhaps you are running outdated dependencies, and this is a bug that has since been fixed, that might explain why I cannot reproduce this.

@Bardiamist
Copy link

Bardiamist commented Oct 31, 2023

base-64 package LGTM, I'll use it
core-js is big

@jonkoops
Copy link
Contributor

Just be aware that base-64 has not been maintained since October 2020. It probably doesn't matter for such a small dependency, but still.

@Bardiamist
Copy link

@Bardiamist
Copy link

@Bardiamist @hrastnik are you able to reproduce the issue in a minimal example like this?

Its difficult, I don't know where is problem. App is completely broken if add import 'core-js/stable/atob'; even if don't call jwtDecode.

@jonkoops
Copy link
Contributor

jonkoops commented Nov 1, 2023

Are you able to reproduce the problem by following the steps under #242 (comment) though? So with a completely clean project?

@Bardiamist
Copy link

Bardiamist commented Nov 1, 2023

It doesn't matter because import 'core-js/stable/atob'; completely breaks something in the app.

Your code

import "core-js/stable/atob";

const message = atob("SGVsbG8sIHdvcmxk");
console.log(message); // "Hello, world"

and

import "core-js/stable/atob";
import { jwtDecode } from "jwt-decode"

const token = "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJmb28iOiJiYXIiLCJleHAiOjEzOTMyODY4OTMsImlhdCI6MTM5MzI2ODg5M30.4-iaDojEVl0pJQMjrbM1EzUIfAZgsbK_kgnVyVxFSVo";
const decoded = jwtDecode(token);

console.log(decoded); // { "exp": 1393286893, "foo": "bar", "iat": 1393268893 }

works without errors if don't render app.

@hrastnik
Copy link

hrastnik commented Nov 1, 2023

I'll add a minimal reproducible example.

@jonkoops
Copy link
Contributor

jonkoops commented Nov 1, 2023

I'll add a minimal reproducible example.

Awesome, that would be much appreciated. I am curious if this is something we perhaps needs to report to the Hermes team.

@hrastnik
Copy link

hrastnik commented Nov 2, 2023

Here's a working example:

import { AppRegistry } from "react-native";
import { name as appName } from "./app.json";
import { jwtDecode } from "jwt-decode";
import { Text, SafeAreaView } from "react-native";
import "core-js/stable/atob";
import "core-js/stable/btoa";

const state = {
  token:
    "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c",
  get decoded() {
    return jwtDecode(this.token);
  },
  get decodedAsString() {
    return JSON.stringify(this.decoded);
  },
};

export const App = function App() {
  return (
    <SafeAreaView style={{ flex: 1, justifyContent: "center" }}>
      <Text style={{ color: "#000000" }}>{state.decodedAsString}</Text>
    </SafeAreaView>
  );
};

AppRegistry.registerComponent(appName, () => App);


...which stops working once you make the state object observable using mobx:

import { AppRegistry } from "react-native";
import { name as appName } from "./app.json";
import { jwtDecode } from "jwt-decode";
import { observable } from "mobx";
import { Text, SafeAreaView } from "react-native";
import "core-js/stable/atob";
import "core-js/stable/btoa";

const state = observable({
  token:
    "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c",
  get decoded() {
    return jwtDecode(this.token);
  },
  get decodedAsString() {
    return JSON.stringify(this.decoded);
  },
});

export const App = function App() {
  return (
    <SafeAreaView style={{ flex: 1, justifyContent: "center" }}>
      <Text style={{ color: "#000000" }}>{state.decodedAsString}</Text>
    </SafeAreaView>
  );
};

AppRegistry.registerComponent(appName, () => App);

This could also be the case with other similar proxy/getter based state management solutions like valtio (https://github.com/pmndrs/valtio), but I haven't tried it.

@Bardiamist
Copy link

This could also be the case with other similar proxy/getter based state management solutions like valtio

I also thought that import 'core-js/stable/atob'; breaks redux but not sure.
@reduxjs/toolkit, proxy-memoize, @react-native-firebase uses Proxy.

@hrastnik
Copy link

hrastnik commented Nov 2, 2023

I think it has to do with core js overwriting/modifying toString method

@Bardiamist
Copy link

core-js package is big ~15MB

@jonkoops
Copy link
Contributor

jonkoops commented Nov 2, 2023

core-js package is big ~15MB

Does all of that get included if you only import core-js/stable/atob? React Native doesn't do Tree Shaking?

@Bardiamist
Copy link

Bardiamist commented Nov 2, 2023

Of course all 15MB will not be bundled but CI should download it. And core-js doing many not requied things when we need one small function. https://github.com/zloirock/core-js/blob/master/packages/core-js/stable/atob.js

@frederikprijck
Copy link
Member Author

frederikprijck commented Nov 2, 2023

It does look like not to be an issue with jwt-decode.

If i understand correctly, corejs' stable atob works but not when combined with mobx, which again is not something we can do anything about and something you want to look into on the RNA/ mobx / corejs side.

Do i understand it correctly that using the pure ponyfill isnt needed neither ? Or would they solve it even with mobx?

Additionally, there are other polyfills or you can even use our old atob and copy it in your project if needed, but we shouldnt ship a polyfill ourselves when the majority supports atob.

@hrastnik
Copy link

hrastnik commented Nov 2, 2023

@frederikprijck Using base-64 package to polyfill atob works without issue even with MobX.

import { encode, decode } from "base-64";
global.atob = decode;
global.btoa = encode;

Additionally, the package is quite small which is nice for reducing download times on the CI.

IMO, it would be nice if this code snippet is listed on the Github page for jwt-decode as it will probably avoid some headaches for RN devs in the future.

@Bardiamist
Copy link

Bardiamist commented Nov 2, 2023

If i understand correctly, corejs' stable atob works but not when combined with mobx

I don't use mobix, I can't detect that exactly broken when import 'core-js/stable/atob'; maybe @reduxjs/toolkit and more.

Would be good if you will recommend best solution in the README.

@hrastnik
Copy link

hrastnik commented Nov 3, 2023

After some investigation, I've found it's a known issue with CoreJS on React Native:

zloirock/core-js#1237

@jonkoops
Copy link
Contributor

jonkoops commented Nov 5, 2023

I am still not able to reproduce the issues in this thread, even adding MobX to the equation. Here is the code I am running:

import "core-js/stable/atob";
import { jwtDecode } from "jwt-decode";
import { observable } from "mobx";
import { SafeAreaView, Text } from "react-native";

const state = observable({
  token: "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c",
  get decoded() {
    return jwtDecode(this.token);
  },
  get decodedAsString() {
    return JSON.stringify(this.decoded);
  },
});

export default function App() {
  return (
    <SafeAreaView style={{ flex: 1, justifyContent: "center" }}>
      <Text style={{ color: "#000000" }}>{state.decodedAsString}</Text>
    </SafeAreaView>
  );
}

@Bardiamist @hrastnik does the comment in facebook/hermes#135 (comment) perhaps solve the issue for you? Or the other suggestions in that thread.

If anyone is interested in making this issue reproducible, perhaps download my project and see if you can modify it so it breaks.

@frederikprijck frederikprijck enabled auto-merge (squash) November 13, 2023 10:08
README.md Outdated Show resolved Hide resolved
frederikprijck and others added 2 commits November 13, 2023 13:22
@frederikprijck frederikprijck merged commit 7d9c30c into main Nov 14, 2023
5 checks passed
@frederikprijck frederikprijck deleted the frederikprijck-patch-1 branch November 14, 2023 09:33
@jonkoops
Copy link
Contributor

Good news, the Hermes team has decided to implement this functionality, so polyfilling these APIs should no longer be needed in the future for React Native (see facebook/hermes#1178 (comment)).

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.

5 participants