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

fix(remix): Automatically infer path to where component is mounted #3104

Conversation

octoper
Copy link
Member

@octoper octoper commented Apr 3, 2024

Description

This PR allows to automatically infer the path for where the component is mounted.

This functionality is already introduced for @clerk/nextjs (#2634)

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

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

Copy link

changeset-bot bot commented Apr 3, 2024

🦋 Changeset detected

Latest commit: d87a2f0

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

This PR includes changesets to release 1 package
Name Type
@clerk/remix 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

@octoper octoper self-assigned this Apr 3, 2024
@octoper
Copy link
Member Author

octoper commented Apr 3, 2024

!snapshot

@clerk-cookie
Copy link
Collaborator

Hey @octoper - the snapshot version command generated the following package versions:

Package Version
@clerk/backend 1.0.1-snapshot.vc39d42e
@clerk/chrome-extension 1.0.1-snapshot.vc39d42e
@clerk/clerk-js 5.0.1-snapshot.vc39d42e
@clerk/clerk-expo 1.0.1-snapshot.vc39d42e
@clerk/fastify 1.0.1-snapshot.vc39d42e
gatsby-plugin-clerk 5.0.1-snapshot.vc39d42e
@clerk/localizations 2.0.1-snapshot.vc39d42e
@clerk/nextjs 5.0.1-snapshot.vc39d42e
@clerk/clerk-react 5.0.1-snapshot.vc39d42e
@clerk/remix 4.0.1-snapshot.vc39d42e
@clerk/clerk-sdk-node 5.0.1-snapshot.vc39d42e
@clerk/shared 2.0.1-snapshot.vc39d42e
@clerk/themes 2.0.1-snapshot.vc39d42e
@clerk/types 4.0.1-snapshot.vc39d42e

Tip: Use the snippet copy button below to quickly install the required packages.
@clerk/backend

npm i @clerk/backend@1.0.1-snapshot.vc39d42e --save-exact

@clerk/chrome-extension

npm i @clerk/chrome-extension@1.0.1-snapshot.vc39d42e --save-exact

@clerk/clerk-js

npm i @clerk/clerk-js@5.0.1-snapshot.vc39d42e --save-exact

@clerk/clerk-expo

npm i @clerk/clerk-expo@1.0.1-snapshot.vc39d42e --save-exact

@clerk/fastify

npm i @clerk/fastify@1.0.1-snapshot.vc39d42e --save-exact

gatsby-plugin-clerk

npm i gatsby-plugin-clerk@5.0.1-snapshot.vc39d42e --save-exact

@clerk/localizations

npm i @clerk/localizations@2.0.1-snapshot.vc39d42e --save-exact

@clerk/nextjs

npm i @clerk/nextjs@5.0.1-snapshot.vc39d42e --save-exact

@clerk/clerk-react

npm i @clerk/clerk-react@5.0.1-snapshot.vc39d42e --save-exact

@clerk/remix

npm i @clerk/remix@4.0.1-snapshot.vc39d42e --save-exact

@clerk/clerk-sdk-node

npm i @clerk/clerk-sdk-node@5.0.1-snapshot.vc39d42e --save-exact

@clerk/shared

npm i @clerk/shared@2.0.1-snapshot.vc39d42e --save-exact

@clerk/themes

npm i @clerk/themes@2.0.1-snapshot.vc39d42e --save-exact

@clerk/types

npm i @clerk/types@4.0.1-snapshot.vc39d42e --save-exact

@octoper octoper requested review from nikosdouvlis and dimkl April 3, 2024 15:15
@octoper octoper marked this pull request as ready for review April 3, 2024 15:20
@octoper octoper force-pushed the vaggelis/sdk-1473-infer-the-path-to-the-component-automatically-for-remix branch from e97b394 to f67ad30 Compare April 4, 2024 11:24
@dimkl
Copy link
Contributor

dimkl commented Apr 4, 2024

@octoper what does the path of useLocation() return for wildcard routes? We need to get part before the wildcard param and use that as path to the component.

@octoper
Copy link
Member Author

octoper commented Apr 4, 2024

@dimkl I have tested that and it resolves the full pathname as is in the browser so it seems there is no need to handle anything in that case.

Correction for my previous statement:
Talked with @dimkl async, and indeed I will have to apply some more updates to correctly handle wildcard routes

@octoper octoper marked this pull request as draft April 4, 2024 20:12
@octoper octoper marked this pull request as ready for review April 4, 2024 21:19
@octoper octoper force-pushed the vaggelis/sdk-1473-infer-the-path-to-the-component-automatically-for-remix branch 2 times, most recently from 47c9f5a to cec481e Compare April 5, 2024 10:35
// Remove the splat route param from the pathname
// so we end up with the pathname where the components are mounted at
// eg /user/123/profile/security will return /user/123/profile as the path
const path = pathname.replace(splatRouteParam, '').replace(/\/$/, '').replace(/^\//, '').trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a couple of tests for this? Maybe turn it into a util.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure we need to test something here as the test here is going to test if replace works correctly, the only thing we do here is we are removing a specific part of the pathname, and also remove slashes from the start and the end of the string only if they exist, also we don't have unit tests specifically for Remix right now, and this part is very specific to Remix to make it a general util. The only thing I see here we can extract in a util and test it is the part of removing slashes.

@octoper octoper force-pushed the vaggelis/sdk-1473-infer-the-path-to-the-component-automatically-for-remix branch from cec481e to d87a2f0 Compare April 5, 2024 12:54
@nikosdouvlis nikosdouvlis merged commit de90d9b into main Apr 15, 2024
10 checks passed
@nikosdouvlis nikosdouvlis deleted the vaggelis/sdk-1473-infer-the-path-to-the-component-automatically-for-remix branch April 15, 2024 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants