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.

Sign up for GitHub

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

Fixes undefined config variables in standalone builds leading to crashes #5

Closed
wants to merge 3 commits into from
+5,799 −3,949
Closed

Fixes undefined config variables in standalone builds leading to crashes #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eb06edd
fix: prod environment variables
didley Nov 8, 2024
5f5e439
Implements own publish script
didley Nov 8, 2024
3807cb8
fix: `expo export` script failing for web
didley Nov 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .npmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
@bearmore:registry=https://npm.pkg.github.com
//npm.pkg.github.com/:_authToken="${GITHUB_TOKEN}"
16 changes: 0 additions & 16 deletions .release-it.json
View file
Open in desktop

This file was deleted.

29 changes: 21 additions & 8 deletions lib/KindeAuthProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,20 +29,33 @@ export const KindeAuthContext = createContext<KindeAuthHook | undefined>(
undefined,
);

// Polyfill for atob
global.btoa = encode;
global.atob = decode;
// global is unavailable for web `expo export` script
if (typeof global !== "undefined") {
// Polyfill for atob
global.btoa = encode;
global.atob = decode;
}

export const KindeAuthProvider = ({
children,
config,
}: {
children: React.ReactNode;
config: {
domain: string | undefined;
clientId: string | undefined;
scopes?: string;
};
Comment on lines +41 to +48
Copy link

@coderabbitai coderabbitai bot Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Strengthen type safety for config prop.

The current type definition allows undefined values for required fields, which doesn't align with the runtime checks. Consider updating the type definition to enforce required values at compile-time:

  config: {
-   domain: string | undefined;
-   clientId: string | undefined;
+   domain: string;
+   clientId: string;
    scopes?: string;
  };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
config,
}: {
children: React.ReactNode;
config: {
domain: string | undefined;
clientId: string | undefined;
scopes?: string;
};
config,
}: {
children: React.ReactNode;
config: {
domain: string;
clientId: string;
scopes?: string;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

   domain: string | undefined;
   clientId: string | undefined;

I feel this typing is correct as they are being asserted internally now and many users will be passing process.env values which will always be string | undefined.

Copy link

Choose a reason for hiding this comment

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

@didley, thank you for the clarification. I understand that users may pass process.env values, which can be string | undefined, so keeping the typings as string | undefined is appropriate. I've noted this for future reviews.

✏️ Learnings added 
Learnt from: didley
PR: kinde-oss/expo#5
File: lib/KindeAuthProvider.tsx:38-45
Timestamp: 2024-11-08T06:21:44.501Z
Learning: In `lib/KindeAuthProvider.tsx`, for the `KindeAuthProvider` component, the `config.domain` and `config.clientId` properties should be typed as `string | undefined` because users may pass `process.env` values, which can be `undefined`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

}) => {
const domain = process.env.EXPO_PUBLIC_KINDE_DOMAIN!;
const clientId = process.env.EXPO_PUBLIC_KINDE_CLIENT_ID!;
const scopes =
process.env.EXPO_PUBLIC_KINDE_SCOPES?.split(" ") ||
DEFAULT_TOKEN_SCOPES.split(" ");
const domain = config.domain;
if (domain === undefined)
throw new Error("KindeAuthProvider config.domain prop is undefined");

const clientId = config.clientId;
if (clientId === undefined)
throw new Error("KindeAuthProvider config.clientId prop is undefined");

const scopes = config.scopes?.split(" ") || DEFAULT_TOKEN_SCOPES.split(" ");

const [isAuthenticated, setIsAuthenticated] = useState<boolean>(false);
const redirectUri = makeRedirectUri({ native: Constants.isDevice });
Expand Down
17 changes: 8 additions & 9 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
{
"name": "@kinde/expo",
"repository": {
"type": "git",
"url": "git+https://github.com/kinde-oss/expo.git"
},
"name": "@bearmore/kinde-expo",
"private": false,
"type": "module",
"files": [
"dist"
],
"version": "0.1.1",
"version": "0.1.10",
"scripts": {
"dev": "vite",
"build": "tsc && vite build",
Expand All @@ -18,7 +14,9 @@
"test": "vitest",
"test:coverage": "vitest --coverage",
"lint": "prettier --check .",
"lint:fix": "prettier --write ."
"lint:fix": "prettier --write .",
"publish-npm": "pnpm build && npm publish",
"publish-dry": "pnpm publish-npm --dry-run"
},
"module": "dist/kinde-expo.cjs",
"main": "dist/kinde-expo.cjs",
Expand All @@ -45,7 +43,8 @@
"expo-auth-session": "^5.5.2",
"expo-crypto": "^13.0.2",
"expo-linking": "^6.3.1",
"expo-secure-store": "^13.0.2"
"expo-secure-store": "^13.0.2",
"expo-web-browser": "^13.0.3"
},
"peerDependencies": {
"expo": "*",
Expand All @@ -55,4 +54,4 @@
"access": "public"
},
"packageManager": "[email protected]+sha256.2df78e65d433d7693b9d3fbdaf431b2d96bb4f96a2ffecd51a50efe16e50a6a8"
}
}
Loading