Refactor createLoginUrl() to use standardized URL APIs
#71
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closes keycloak#70 Signed-off-by: Jon Koops <jonkoops@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the createLoginUrl() function to use standardized URL APIs and updates related tests and support files accordingly.
- Updated type imports in tests from keycloak.js to keycloak.d.ts
- Added comprehensive tests for various login URL options in login-url.spec.ts and removed redundant claims tests
- Refactored createLoginUrl() in lib/keycloak.js to use URLSearchParams and modern syntax, and updated the TestBed and TestExecutor interfaces/files
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/tests/silent-sso.spec.ts | Updated type import to use keycloak.d.ts |
| test/tests/scope.spec.ts | Updated type import to use keycloak.d.ts |
| test/tests/login.spec.ts | Updated type import to use keycloak.d.ts |
| test/tests/login-url.spec.ts | Added new tests to verify comprehensive login URL behaviors |
| test/tests/implicit-flow.spec.ts | Updated type import to use keycloak.d.ts |
| test/tests/claims.spec.ts | Removed redundant claims tests |
| test/support/testbed.ts | Added realm property to TestBed interface and return value |
| test/support/test-executor.ts | Updated createLoginUrl signature to accept an optional parameter |
| lib/keycloak.js | Refactored createLoginUrl() to use URLSearchParams and modern syntax |
Comments suppressed due to low confidence (2)
test/support/testbed.ts:92
- [nitpick] Now that 'realm' is added to the TestBed interface and returned in createTestBed, consider adding a brief inline comment explaining this new property for maintainability.
+ realm,
lib/keycloak.js:497
- Review the removal of the second parameter from adapter.redirectUri(options); ensure this change does not alter the intended behavior for logout redirects, particularly if a false flag was used to disable any encoding or special handling previously.
+ + '&post_logout_redirect_uri=' + encodeURIComponent(adapter.redirectUri(options));
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #70