Fix and Migrate Version URL Parser #3801
Conversation
Updated parseVersionString to handle object format instead of assuming string format.
- Add regression tests for major/major.minor version patterns - Add tests for invalid input fallback behavior - Migrate test file to TypeScript - Export p5VersionStrings from parseUrlParams for test validation - Use regex assertions to avoid breaking when p5Versions updates
|
🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. |
client/utils/parseURLParams.ts
Outdated
| return typeof item === 'string' ? item : item.version; | ||
| } | ||
|
|
||
| export const p5VersionStrings = p5Versions.map(getVersionString); |
There was a problem hiding this comment.
Is this being called anywhere else? Otherwise we can probably keep it local and not export it
There was a problem hiding this comment.
It's only used in the parseURLParams.test.ts file. I can move it to be local if preferred. It's a small bit of code so I wasn't sure if I should avoid code duplication or not.
There was a problem hiding this comment.
Nice! thanks for doing this
| } | ||
|
|
||
| export function parseUrlParams(url) { | ||
| export function parseUrlParams(url: string): ParsedUrlParams { |
There was a problem hiding this comment.
Could we add JSDocs for what this function is for?
|
Looks great, thanks for migrating this! Just some small comments 🙏 |
clairep94
added
the
Area: Typescript Migration
client/utils/parseURLParams.test.ts
Outdated
| const bad = parseUrlParams('https://example.com?version=9.9.9'); | ||
| expect(bad.version).toBe(currentP5Version); |
There was a problem hiding this comment.
The invalid-version test relies on '9.9.9' never becoming a real release. Once p5 publishes that tag, the test will report a failure even though the fallback logic works. Using an obviously invalid token (e.g., 'invalid-version' or a value derived from p5Versions that is guaranteed to be absent) would make the check resilient to future releases.
There was a problem hiding this comment.
Good point. I didn't think about future releases breaking this. I'll update it to use 'invalid-version'.
| function getNewestVersion(versions) { | ||
| function getVersionString( | ||
| item: string | { version: string; label: string } | ||
| ): string { |
There was a problem hiding this comment.
Looks like it can also resolve #3779. Can you help double check it?
There was a problem hiding this comment.
Yes the issue is resolved by this.
- Add JSDoc to parseUrlParams - Remove export from p5VersionStrings and copy its logic to test file - Fix invalid version test to use 'invalid-version' instead of '9.9.9'
3 participants
Fixes #3761
Fixes #3762
Changes
Testing
Testing Strategy Note
The version parsing tests use regex patterns (e.g.,
/^1\.\d+\.\d+$/) rather than hardcoded expected versions (e.g.,'1.11.11'). This approach:p5VersionStringsAlternative approach: Tests could assert exact versions (e.g.,
expect(result.version).toBe('1.11.11')), which would catch sorting bugs but require updates wheneverp5Versionschanges.Sorting bugs are very unlikely to occur unless version numbers change drastically. Current tests simply attempt to prevent future silent errors.
I'm happy to switch to hardcoded assertions or explore other testing strategies if you have suggestions!
I have verified that this pull request:
npm run lint)npm run test)npm run typecheck)developbranch.Fixes #123