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

Update private routes and Set to use roles vs role #4681

Merged
merged 5 commits into from
Mar 16, 2022

Conversation

dthyresson
Copy link
Contributor

@dthyresson dthyresson commented Mar 7, 2022

Fix #4488

Purpose

  1. Pluralize role in the Router prop
  2. Deprecate role prop so as to not make this a breaking change.

I could not figure out how to deprecate and still keep the logic in the router to take a role or roles.

But I do't think we should allow role since that is used by route announcements and it would be more confusing to use role differently in two different contexts:

            <div
              id="redwood-announcer"
              style={{
                position: 'absolute',
                top: 0,
                width: 1,
                height: 1,
                padding: 0,
                overflow: 'hidden',
                clip: 'rect(0, 0, 0, 0)',
                whiteSpace: 'nowrap',
                border: 0,
              }}
              role="alert"
              aria-live="assertive"
              aria-atomic="true"
              ref={announcementRef}
            ></div>
          )}

Here is is a route alert.

@dthyresson dthyresson added release:breaking This PR is a breaking change release:fix This PR is a fix labels Mar 7, 2022
@dthyresson dthyresson removed the release:fix This PR is a fix label Mar 14, 2022
@dthyresson dthyresson requested a review from cannikin March 14, 2022 14:29
@cannikin
Copy link
Member

So this does not deprecate role correct, it's a hard change-over? What happens with an existing app with role when you upgrade and start the dev server? Does it just fail silently or do you see a message about an unknown prop or something? Maybe rather than emulating the old behavior, it can accept both role and roles but if you pass role it at least throws an error that you can see in the console? The error can even say "use roles now."

@thedavidprice should weigh in on this, since it'll effect basically every Redwood app that exists!

@dthyresson
Copy link
Contributor Author

dthyresson commented Mar 14, 2022

@thedavidprice should weigh in on this, since it'll effect basically every Redwood app that exists!

Agreed. BUT! remember that role singular is already used for other things like accessibility and route announcements.

@thedavidprice
Copy link
Contributor

I'm ok with a hard change. We just need to do it now.

If hard change, this would be a really really nice to have:

Maybe rather than emulating the old behavior, it can accept both role and roles but if you pass role it at least throws an error that you can see in the console? The error can even say "use roles now."

Code mode would be great. Just not sure if it's feasible given all the things.

Either way, I need Release Notes copy. And docs will need updating, true?

@dthyresson
Copy link
Contributor Author

@thedavidprice or @cannikin can I get a review? Thanks!

@thedavidprice
Copy link
Contributor

@cannikin please do help with the review. Frankly I don't know which role is which plural... 🤷‍♂️

@thedavidprice thedavidprice merged commit 5291913 into redwoodjs:main Mar 16, 2022
@jtoar jtoar added this to the next-release milestone Mar 16, 2022
@dac09
Copy link
Contributor

dac09 commented Mar 17, 2022

I’ll work on the codemod for this!

@thedavidprice thedavidprice modified the milestones: next-release, v0.50.0 Mar 23, 2022
@chenliu9 chenliu9 mentioned this pull request Apr 2, 2022
jtoar pushed a commit that referenced this pull request Apr 2, 2022
1. `role` => `roles`. `role` was replaced by `roles` in #4681
2. updated user role example.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:breaking This PR is a breaking change
Projects
No open projects
Status: Archived
Development

Successfully merging this pull request may close these issues.

Disparity of singular vs. plural role and roles in Router vs auth.js
5 participants