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

chore(shared): Exports match utility from the path-to-regexp lib #4187

Merged
merged 7 commits into from
Sep 19, 2024

Conversation

izaaklauer
Copy link
Contributor

@izaaklauer izaaklauer commented Sep 18, 2024

Description

What changed?

This surfaces a new path-to-regexp function, match, that upcoming functionality depends on.

How did you generate the minified js?

Thanks to @nikosdouvlis for giving me the run-down. The procedure goes:

To create the new minified code (high-level)

  • Create a minimal typescript project with an index.ts that imports the functionality that you need
  • Use tsup to bundle it into a minified file
  • Beautify the minified file (e.g. add newlines)

More details for internal folks here: https://clerkinc.slack.com/archives/C07LN8YS9HB/p1726666913614259?thread_ts=1726240424.533199&cid=C07LN8YS9HB

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Copy link

changeset-bot bot commented Sep 18, 2024

🦋 Changeset detected

Latest commit: 061c555

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages
Name Type
@clerk/shared Patch
@clerk/astro Patch
@clerk/backend Patch
@clerk/chrome-extension Patch
@clerk/clerk-js Patch
@clerk/elements Patch
@clerk/clerk-expo Patch
@clerk/express Patch
@clerk/fastify Patch
@clerk/nextjs Patch
@clerk/clerk-react Patch
@clerk/remix Patch
@clerk/clerk-sdk-node Patch
@clerk/tanstack-start Patch
@clerk/testing Patch

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

Comment on lines 28 to 31
/**
* A match is either `false` (no match) or a match result.
*/
export type Match<P extends object = object> = false | MatchResult<P>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pulled in the input and output types for match, and all the types they depend on.

@izaaklauer izaaklauer changed the title Izaak/orgs 132 add path to regexp match chore(shared): add path-to-regexp match Sep 18, 2024
.changeset/big-elephants-add.md Outdated Show resolved Hide resolved
packages/shared/src/pathToRegexp.ts Outdated Show resolved Hide resolved
@LauraBeatris
Copy link
Member

LauraBeatris commented Sep 18, 2024

On another note, I wonder if we could import the types into the compiled version - in the Next.js repo I see types being exported without needing to be redeclared.

izaaklauer and others added 2 commits September 18, 2024 12:50
Co-authored-by: Laura Beatris <48022589+LauraBeatris@users.noreply.github.com>
Specifically, running `tsup` with the `--dts-resolve` flag, and then copying over the .d. file
@izaaklauer
Copy link
Contributor Author

On another note, I wonder if we could import the types into the compiled version - in the Next.js repo I see types being exported without needing to be redeclared.

Good idea - I found the --dts-resolve flag on tsup, which generated the type declarations that we need, then I copied over those types.

deb5b9d

I think the pre-commit hook stripped these, so i'm re-adding them so as not to create an unnecessary diff, but I don't think they're strictly necessary?
} catch (e: any) {
throw new Error(
`Invalid path: ${path}.\nConsult the documentation of path-to-regexp here: https://github.com/pillarjs/path-to-regexp\n${e.message}`,
`Invalid path: ${path}.\nConsult the documentation of path-to-regexp here: https://github.com/pillarjs/path-to-regexp/tree/6.x\n${e.message}`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling this out: I think it's more useful to point people toward the docs for the specific version that we've embedded, because the syntax on latest is different.

Copy link
Member

@nikosdouvlis nikosdouvlis Sep 19, 2024

Choose a reason for hiding this comment

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

Good point!

Copy link
Member

@nikosdouvlis nikosdouvlis left a comment

Choose a reason for hiding this comment

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

LGTM :)

@LauraBeatris LauraBeatris changed the title chore(shared): add path-to-regexp match chore(shared): Add path-to-regexp match Sep 19, 2024
@LauraBeatris LauraBeatris changed the title chore(shared): Add path-to-regexp match chore(shared): Exports match utility from the path-to-regexp lib Sep 19, 2024
@izaaklauer izaaklauer merged commit cb32aaf into main Sep 19, 2024
22 checks passed
@izaaklauer izaaklauer deleted the izaak/ORGS-132-add-path-to-regexp-match branch September 19, 2024 14:59
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.

3 participants