-
Notifications
You must be signed in to change notification settings - Fork 120
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
Add lint hardcoded URLs in JavaScript #10894
Add lint hardcoded URLs in JavaScript #10894
Conversation
changelog: Internal, Continuous Integration, Add JavaScript lint to prevent hardcoding URLs
f09363c
to
22c7f09
Compare
@@ -52,8 +52,6 @@ interface SessionTimedOutStatus { | |||
|
|||
export type SessionStatus = SessionLiveStatus | SessionTimedOutStatus; | |||
|
|||
export const SESSIONS_URL = new URL('/api/internal/sessions', window.location.href).toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API endpoints are more consistent than the ones that have locales in them, so it could have been fine if we wanted right? This is just for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The session endpoints don't include any user-facing language currently so there aren't any issues right now, so passing in the URL is just for the localization. The other upside is we get path validation with Rails.
I"m not sure I understand what you mean by API endpoints being more consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consistent meaning they don't change based on locale param
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, got it. I think generally, yes, but there are cases like the country support endpoint that return translated content depending on the requested locale.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had originally knowingly implemented this without the path helper based on the original comment that we don't need the endpoint to be localized, but agree it's nice to use path helpers for validation and having a single source of truth for routes. And avoids having to create an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from discussion of capitalization, LGTM 👍
Co-authored-by: Zach Margolis <[email protected]>
Co-authored-by: Zach Margolis <[email protected]>
* Lint hard-coded URLs in JavaScript changelog: Internal, Continuous Integration, Add JavaScript lint to prevent hardcoding URLs * session timeout * address search URL * Update app/javascript/packages/session/requests.ts Co-authored-by: Zach Margolis <[email protected]> * Update app/javascript/packs/session-timeout-ping.ts Co-authored-by: Zach Margolis <[email protected]> * rename variables --------- Co-authored-by: Andrew Duthie <[email protected]> Co-authored-by: Zach Margolis <[email protected]>
* Lint hard-coded URLs in JavaScript changelog: Internal, Continuous Integration, Add JavaScript lint to prevent hardcoding URLs * session timeout * address search URL * Update app/javascript/packages/session/requests.ts Co-authored-by: Zach Margolis <[email protected]> * Update app/javascript/packs/session-timeout-ping.ts Co-authored-by: Zach Margolis <[email protected]> * rename variables --------- Co-authored-by: Andrew Duthie <[email protected]> Co-authored-by: Zach Margolis <[email protected]>
🛠 Summary of changes
Builds on work from @aduth and learnings from #10882 where hardcoded strings for URLs resulted in untranslated requests from JavaScript. This lint raises errors on
new URL(string)
to help mitigate against similar issues in the future.address_search_url
is not used currently due to current configurations, the functionality for"/api/addresses"
was removed in #9154. The replacement with an empty string is intended to serve as a placeholder while discussion on whether to remove the frontend components of the address search happens here.