Add beta-switch package #1263
Add beta-switch package #1263
Conversation
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
packages/beta-switch/src/index.ts
Outdated
| const desireCookie = new RegExp("betaDesire=([^;]+);").exec( | ||
| document.cookie[-1] === ";" ? document.cookie : document.cookie + ";" | ||
| )?.[1]; |
There was a problem hiding this comment.
JavaScript does not support negative array indexing. To check if the last character is a semicolon, replace document.cookie[-1] with document.cookie[document.cookie.length - 1]. This will properly access the last character of the string.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
packages/beta-switch/src/index.ts
Outdated
| enabledPages.forEach((regPath) => { | ||
| const regExecuted = new RegExp(regPath).exec(window.location.pathname); | ||
| const found = regExecuted !== null && regExecuted.length < 2; | ||
| if (!betaIsAvailable && found) { | ||
| betaIsAvailable = true; | ||
| } | ||
| }, betaIsAvailable); |
There was a problem hiding this comment.
The forEach has a second argument betaIsAvailable passed as thisArg which appears unintentional and could lead to bugs. The thisArg parameter is meant to set the value of this inside the callback function, but isn't needed here. Consider removing the second argument to forEach.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
packages/beta-switch/package.json
Outdated
| "main": "dist/thunderstore-beta-switch.cjs.js", | ||
| "module": "dist/thunderstore-beta-switch.esm.js", | ||
| "types": "dist/thunderstore-beta-switch.cjs.d.ts", |
There was a problem hiding this comment.
There appears to be a mismatch between the output paths specified in package.json and TypeScript's actual output. TypeScript's tsc command will generate different file names than what's listed here. To resolve this, either:
- Add a bundler (like Rollup or esbuild) to generate the
.cjs.jsand.esm.jsfiles, or - Update these paths to match TypeScript's output format (typically
.jsand.d.tsfiles)
This will ensure the built files are available at the locations the package declares.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
Oksamies
force-pushed
the
11-13-add_beta-switch_package
branch
from
2f4a88e to
a8ea507
Compare
| async function getCookie(key) { | ||
| return new RegExp(`${key}=([^;]+);`).exec( | ||
| document.cookie[-1] === ";" ? document.cookie : document.cookie + ";" | ||
| )?.[1]; | ||
| } |
There was a problem hiding this comment.
The document.cookie[-1] check is unsafe since document.cookie may be empty. A simpler and more robust approach would be to always append the semicolon: document.cookie + ';'. This eliminates the need for the conditional and handles empty cookie cases gracefully.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
| async function setCookie(key, value) { | ||
| const date = new Date(); | ||
| date.setFullYear(date.getFullYear() + 1); | ||
| document.cookie = `${key}=${value}; expires=${date.toUTCString()}; path=/`; | ||
| } |
There was a problem hiding this comment.
The cookie needs a domain attribute to enable sharing between subdomains. Add ; domain=.thunderstore.temp to the cookie string to allow both thunderstore.temp and new.thunderstore.temp to access it.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
Oksamies
force-pushed
the
11-13-add_beta-switch_package
branch
from
a8ea507 to
430cecc
Compare
| @@ -8,6 +8,7 @@ events { | |||
| http { | |||
| server { | |||
| listen 80; | |||
| server_name thunderstore.temp | |||
There was a problem hiding this comment.
The server_name directive is missing a semicolon, which will cause nginx to fail on startup. The correct syntax is:
server_name thunderstore.temp;Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
Oksamies
force-pushed
the
11-13-add_beta-switch_package
branch
from
430cecc to
29e7b60
Compare
No description provided.