-
Notifications
You must be signed in to change notification settings - Fork 244
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
feat(clerk-js): Add bundle-check script #4199
Conversation
🦋 Changeset detectedLatest commit: 8534ed0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
||
function signIn() { | ||
const script = ` | ||
window.Clerk.load({ router: window.VIRTUAL_ROUTER }).then(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This router
option is not actually used in this version of clerk-js
. It's being introduced in #4114 but it doesn't hurt anything to have it here now. Happy to remove it if it gives anyone heartburn!
packages/clerk-js/bundle-check.mjs
Outdated
<script | ||
type="text/javascript" | ||
src="/clerk.browser.js" | ||
data-clerk-publishable-key="${process.env.CLERK_PUBLISHABLE_KEY}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CLERK_PUBLISHABLE_KEY
is not exported from some frameworks such as Next.js, right? In this case, would be NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY
- how do we forward it to clerk-js?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This script is purely for our own use, and doesn't come into play for any runtime behaviors. We'll be manually running it like node bundle-check.mjs
. I honestly want to just embed a key into the file, but we've never done that (and the few that are embedded point to lclclerk.dev
domains, so the AIO components won't render with them). I don't know of a reason why embedding a publishable key would be bad, but I wanted to get confirmation before I did that. I would strongly prefer we not have to set an environment variable to run the script.
if (req.url && req.url in routes) { | ||
res.writeHead(200, { 'content-type': 'text/html' }); | ||
res.end(routes[req.url]); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not checking if req.url
is defined here - It might be an impossible case to lead to issues since fs.existsSync(filePath)
would fail, but maybe it's better to safeguard for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is actually because the TypeScript types are not great. req.url
is always defined for incoming messages received by the HTTP server. But I agree that there's no harm in still checking! I'll add that.
Co-authored-by: Laura Beatris <48022589+LauraBeatris@users.noreply.github.com>
Description
This PR adds a new script,
bundle-check.mjs
, toclerk-js
. This allows us to confirm which chunks are dynamically loaded during a particular invocation of Clerk, and to see their gzipped sizes. This will be useful when comparing the total loaded JavaScript sizes of the new versus the old components when they're both in theclerk-js
package.Example output:
Checklist
npm test
runs as expected.npm run build
runs as expected.Type of change