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

Slight decouple from react-router-dom@5 #188

Closed
wants to merge 1 commit into from

Conversation

EthanStandel
Copy link
Contributor

fix: allow dependents to utilize react-router-dom@latest with less specific import

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Adding Tests
  • Build related changes
  • CI related changes
  • Documentation changes
  • Other... Please describe:

What is the current behavior?

When users currently try to utilize react-router-dom@latest, the SecureRoute component imports fail builds because it relies on react-router-dom@5 SDK. This happens even if users aren't utilizing this component because this package is exported as a single rollup.

Issue Number: 176, 177

What is the new behavior?

Use import * as ReactRouterDom and then utilize possibly null properties. This brings the use of the removed useRouteMatch hook to a runtime check rather than expecting it to exist at build time.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Tried to make this as low risk as possible. It's also worth noting that the RouteProps type has to exist on the import at build-time. Thankfully, react-router-dom@latest currently does export a type by this name. If they change that in the future then the problem will return. But for now it's all good. Either way that issue doesn't break any case that isn't already not working.

Reviewers

@denysoblohin-okta
Copy link
Contributor

Thanks for submitting this PR.
Changes makes sense. We'll work on supporting react-router v5 and v6.
Internal ref: OKTA-318565

@maria-mcparland
Copy link

@denysoblohin-okta is there a timeline for when react-router v6 support will be released?

@freakyfelt
Copy link

freakyfelt commented Dec 3, 2021

Hey y'all until v6 support is out can we please merge and release this? TypeScript refuses to compile because of this specific import

@EthanStandel
Copy link
Contributor Author

@denysoblohin-okta Any chance we could use this as something like 6.3.1-beta or something for now?

@PakitoSec
Copy link

@denysoblohin-okta what is the status of supporting react-router v6 ?

If we import Security component we have this error:

in ./node_modules/@okta/okta-react/bundles/okta-react.esm.js 284:14-27
export 'useRouteMatch' (imported as 'useRouteMatch') was not found in 'react-router-dom'

@jaredperreault-okta
Copy link
Contributor

Merged in 4f6f52f

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.

None yet

6 participants