ensure suggested app names are valid #5293
Conversation
|
We detected some changes at packages/*/src and there are no updates in the .changeset. |
Coverage report
Show files with reduced coverage 🔻
Test suite run success2032 tests passing in 909 suites. Report generated by 🧪jest coverage report action from 4a04aa9 |
| @@ -479,14 +484,15 @@ interface GenerateRandomDirectoryOptions { | |||
| */ | |||
| export async function generateRandomNameForSubdirectory(options: GenerateRandomDirectoryOptions): Promise<string> { | |||
| const generated = `${getRandomName(options.family ?? 'business')}-${options.suffix}` | |||
|
|
|||
There was a problem hiding this comment.
Do we want to ensure that we don't end up in loops, perhaps with a fallback name after X attempts? There's recursion here with no base case other than "we will likely generate a name that works".
There was a problem hiding this comment.
Yeah, everything about this PR is off-putting to me (I think I'm allowed to say that because it is my PR lol).
I thought about the case you are describing, but didn't want to introduce too much complexity for this edge case of convenience behavior, which would requiring adding a param for counting recursion depth, and some fallback behavior (probably couldn't be to just truncate in case the filepath for that name is already taken, so a secondary fall back to an empty string), or else having some other heavy implementation like eager avoidance of invalid names.
However, taking a step back, do you think this is the right direction? Here is roughly the landscape I see:
- Rather than the approach of this PR to gracefully handle too-long names, we preclude their possibility: remove the one long noun (
infrastructure) that can alone can cause the name to be greater than 30 characters, extract the max length of an app name (as done in this PR), and add a test which checks that every permutation of the name seeds is not longer than the max length. The resolves the problem this PR sets out to address, and guards against its return (whether from new seed names being added, or the maximum app name length decreasing), with no runtime overhead. However, it introduces over a thousand new assertions and the maintenance costs of these data-validation tests; further if we wanted to swap out this set with some other 1st/3rd party seeds which are larger, we'd have to roll this back and go with option 2. - Keep this PR, and add a base fallback for after X attempts. However this introduces hotpath costs and code complexity to accommodate a scenario which could be prevented ahead of time, and just isn't that bad.
What do you think?
There was a problem hiding this comment.
All good points! Blast radius is low here, so we don't need to spend a ton of time on it. Probably what you have is fine.
However: regardless of what name we generate, the filesystem state is an unknown variable that'll likely need a check. I like your testing idea, but you're right that swapping the generator library might leave us with a long-running suite. Maybe we can make the generator conform to a max character count? And then just leave the existing FS check? Or just how you've got it is probably fine.
There was a problem hiding this comment.
Yeah I'm not sure the retries and validation checks are necessary - overall this is a problem of characters being too long, but the idea of the name being "valid" beyond the character length is done through server-side checks anyways. I think retries make sense when the thing that is valid could regularly change at any given time (i.e. uniqueness checks, handle generation, etc), but this collision could be solved by appending some numbers on the end to avoid the subdirectory collision. It doesn't change the name of the app, but we don't need as much recursion logic 🤷🏻
Maybe we can make the generator conform to a max character count? And then just leave the existing FS check?
These names are purely client-side generated and we can set up the seeds such that they never produce invalid data. As you said, if the seeds change, we can have guards against that.
However, it introduces over a thousand new assertions and the maintenance costs of these data-validation tests
The names seem to be generated deterministically, so if the longest words in the seed of each component don't exceed max character length, is that a sufficient test? You wouldn't need thousands of assertions in that scenario because we don't have to check every permutation
| @@ -20,3 +20,9 @@ function isValidUrl(input: string, httpsOnly: boolean) { | |||
| export function ensurePathStartsWithSlash(arg: unknown) { | |||
| return typeof arg === 'string' && !arg.startsWith('/') ? `/${arg}` : arg | |||
| } | |||
|
|
|||
| export const APP_NAME_MAX_LENGTH = 30 | |||
There was a problem hiding this comment.
Is this value shared with other constraints in the CLI? Or is it a convention we're inheriting from core?
There was a problem hiding this comment.
This value had never been extracted (as far as I could tell). I extracted it to get the uses I found of this validation downstream from this single source of truth, rather than as disparate magic numbers (in dev.ts and wherever the app name length validation happens).
craigmichaelmartin
force-pushed
the
ensure-suggested-app-names-are-valid
branch
2 times, most recently
from
77b0734 to
8425c1a
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/fs.d.ts@@ -254,6 +254,10 @@ interface GenerateRandomDirectoryOptions {
directory: string;
/** Type of word to use for random name. */
family?: RandomNameFamily;
+ /** Used to to track how many attempts have been made to generate the name. */
+ attempts?: number;
+ /** Predicate for validating the generated name. */
+ isValidName?: (name: string) => boolean;
}
/**
* It generates a random directory directory name for a sub-directory.
|
| /** Used to to track how many attempts have been made to generate the name. */ | ||
| attempts?: number | ||
|
|
||
| /** Predicate for validating the generated name. */ | ||
| isValidName?: (name: string) => boolean |
There was a problem hiding this comment.
i'm not sure about this, the function should be simple, just generate a name following some options (suffix, family and the directory).
It should be responsibility of the caller to validate the proposed name and decide whether or not to try again (and how many times).
attemps should definitely not be part of the options if you want to use it to track the recursion, because it can be set externally and any caller will see this and interpret it as "max attemps" and set a number.
My proposal:
- keep
generateRandomNameForSubdirectoryas it is now - Make the app init validate the result name and retry again if needed
- Remove the extra long word, just to be sure.
What do you think?
There was a problem hiding this comment.
Sounds good to me! Thanks, Isaac!
craigmichaelmartin
force-pushed
the
ensure-suggested-app-names-are-valid
branch
from
21ea506 to
22f26de
Compare
craigmichaelmartin
force-pushed
the
ensure-suggested-app-names-are-valid
branch
2 times, most recently
from
cc3ff48 to
be614db
Compare
craigmichaelmartin
force-pushed
the
ensure-suggested-app-names-are-valid
branch
from
be614db to
4a04aa9
Compare
WHY are these changes introduced?
We currently have an app name length limit of 30 characters, but can suggest names longer than 30 characters.
WHAT is this pull request doing?
Retries if the generated name is too long.
Measuring impact
How do we know this change was effective? Please choose one:
Checklist