fix: follow standard pattern for auto-generated site names #5007

tinfoil-knight · 2022-08-28T12:37:43Z

🎉 Thanks for submitting a pull request! 🎉

Summary

The suggestions for auto-generated site names from the CLI don't follow the same pattern as they do when created from the Netlify App. These suggestions also use a very limited number of combinations which results in a user having a lot of site names being very similar & not being able to discern b/w them.

Additionally, the suggested site names can lead to invalid URLs as described here: #4653 (comment)

This PR fixes both the issue with standard names & site length. Also see #5007 (comment)

For us to review and ship your PR efficiently, please perform the following steps:

Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
Read the contribution guidelines 📖. This ensures your code follows our style guide and
passes our tests.
Update or add tests (if any source code was changed or added) 🧪
Update or add documentation (if features were changed or added) 📝
Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

github-actions · 2022-08-28T12:39:11Z

📊 Benchmark results

Comparing with 85f961d

Package size: 227 MB

⬇️ 0.05% decrease vs. 85f961d

^  224 MB  224 MB  224 MB  224 MB  224 MB  224 MB  224 MB  224 MB  224 MB  224 MB  224 MB  227 MB  227 MB 
│   ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-12    T-11    T-10    T-9     T-8     T-7     T-6     T-5     T-4     T-3     T-2     T-1      T

Legend

T-30 (12bb1e1): 223 MB
T-29 (5043eae): 223 MB
T-28 (a7551dd): 223 MB
T-27 (a7c8892): 223 MB
T-26 (a4a0bdb): 223 MB
T-25 (91506bb): 223 MB
T-24 (b9e0917): 223 MB
T-23 (5ace78f): 224 MB
T-22 (fed6676): 224 MB
T-21 (c5d6790): 224 MB
T-20 (567ca4f): 224 MB
T-19 (63acc5e): 224 MB
T-18 (bedabad): 224 MB
T-17 (bc9cd53): 224 MB
T-16 (db9f642): 224 MB
T-15 (58699d4): 224 MB
T-14 (dd1993f): 224 MB
T-13 (254b7c0): 224 MB
T-12 (03f688e): 224 MB
T-11 (a52835b): 224 MB
T-10 (1d809f0): 224 MB
T-9 (986a12d): 224 MB
T-8 (dc0d7f6): 224 MB
T-7 (d91db97): 224 MB
T-6 (d29c7fd): 224 MB
T-5 (b6d80e5): 224 MB
T-4 (cb1df05): 224 MB
T-3 (dd9c209): 224 MB
T-2 (be3d57d): 224 MB
T-1 (85f961d): 227 MB
T (current commit): 227 MB

src/utils/sites/utils.js

tinfoil-knight · 2022-08-28T12:54:15Z

The longest possible site name suggestion is extraordinary-croquembouche-${suffix} where suffix is of length 6 making the maximum length 34.

For deploy previews, a 24 char hash is pre-pended (as per #4653 (comment)) which would make the max length 60 (34 + 24 + 2 for -- separator).

A suggestion should never cross the length of 37 characters (63 - (24 + 2))

In all cases, the URLs stay below the 63 char length which keeps everything functional.

danez · 2022-08-29T12:12:47Z

I have mixed feelings about this. I do not like the code duplication from the server-side. In case we ever decide to change the logic server-side this can create problems again.

What if instead of creating a name on the client we simply do not do this at all? If the API call createSiteInTeam is called without name then the server generates a name. This makes this way more simple and we do not have any code duplication. I quickly tried this by removing all autogeneration and it seems to work fine.

Do you see any problem with this?

tinfoil-knight · 2022-08-29T12:45:21Z

What if instead of creating a name on the client we simply do not do this at all? If the API call createSiteInTeam is called without name then the server generates a name. This makes this way more simple and we do not have any code duplication. I quickly tried this by removing all autogeneration and it seems to work fine.

Do you see any problem with this?

Only issue with this is that it seems like the site name is going to be empty & there's no indication of what it's going to be.

Although, the Netlify UI also does the same thing (assign name without pre-indication).

I'll make the changes if you feel its necessary. Will need to update the CLI prompts a bit to indicate that an auto-generated name will be assigned.

Flow with auto-generated suggestion visible. (Current behaviour)

danez · 2022-08-29T12:57:23Z

I personally feel this is okay if we add a message like leave empty for autogenerated name.

Ping @netlify/developer-experience-members What is your take on this? Any recommendations?

whitep4nth3r · 2022-08-30T07:50:32Z

Something like Press enter to {generate a random site name} where 'Press enter' is the action, rather than 'Leave empty' which is an anti-action (kind of).

I left generate a random site name in curly brackets because I'm not sold on it.

tinfoil-knight · 2022-08-30T09:37:42Z

@whitep4nth3r

How about Skip to let Netlify pick a name for your site ?

The prompt in the CLI would then be:

? Site name (you can change it later): Skip to let Netlify pick a name for your site

whitep4nth3r · 2022-08-30T10:02:59Z

But how does someone Skip? That's not clear. I know we're getting deep into semantics and copy-writing and I'm not sure that's my area of expertise... but I'm not sure Skip instructs the user to press enter/return.

tinfoil-knight · 2022-08-30T10:14:18Z

But how does someone Skip? That's not clear. I know we're getting deep into semantics and copy-writing and I'm not sure that's my area of expertise... but I'm not sure Skip instructs the user to press enter/return.

I didn't want to use Press enter because it seems like an action step without much choice. Skip seems like it offers a choice to let the user not think of the site name (which is the intended use of auto-generated names).

I do agree that Skip might be a bit unclear but pressing enter or return to skip a step is usual behaviour for many CLI utilities (from anecdotal experience) so I don't think this should be too confusing.

whitep4nth3r · 2022-08-30T10:22:23Z

You know, I'm not sure we need a message if the auto-generated name is a suggestion like in the image above from @tinfoil-knight.

I just ran through setting up a new git repo via the GitHub CLI for inspiration. All (lighter contrast) prompts are e.g. (Y/n) or(.origin) where the default option is chosen when you press enter without being told.

Maybe we just need to show the auto generated name without an extra message. That feels more intuitive to me. What do you think?

tinfoil-knight · 2022-08-30T10:28:05Z

Maybe we just need to show the auto generated name without an extra message. That feels more intuitive to me. What do you think?

@whitep4nth3r I like the auto-generated name option from a user's PoV but the issue with this is that it uses code that relies on internal server implementation (Netlify API) of how the site names are generated which makes it brittle which is why @danez suggested to avoid doing this in #5007 (comment)

whitep4nth3r · 2022-08-30T10:34:50Z

Oh I see, that makes sense.

So the options are along the lines of:

Site name (you can change it later): leave empty for autogenerated name
Site name (you can change it later): skip to let Netlify pick a name for your site

I wonder whether we can get a bit of Netlify personality in here. Maybe I'm over-thinking it but there's maybe an opportunity to be a little quirky here given we can't show the auto-generated name.

Along the lines of....?

Site name (you can change it later): pick for me
Site name (you can change it later): I'm feeling lucky

Maybe we have an array of quirky strings to show a random one out of a selection each time? Am I going too far? 😅

tinfoil-knight · 2022-08-30T10:44:34Z

Site name (you can change it later): pick for me sounds okay. I'd avoid going with an array of strings for consistency on the command line.

@danez I'll go forward with this & make changes to the flow to rely on the server for generating names if left empty.

@whitep4nth3r If you come up with any other ideas in the meantime, just let me know.

tinfoil-knight · 2022-09-02T08:16:59Z

Change Summary

Edit: Everything below is outdated now since the default value that displayed Pick for me has been removed.

sites:create will no longer auto-suggest names & will just a Pick for me text. If left as it is, name will be selected by the server.

The sites:create-template command will now use the name of the template if the site name isn't available to create the Github Repository.

If a repository with the same name is already present, the user will get a well defined error.

The site name will be selected by the server if not provided.

The

tinfoil-knight

@danez Let me know if any additional tests are required.

eduardoboucas · 2022-09-05T08:48:55Z

src/commands/sites/sites-create.js

    const { name: nameInput } = await inquirer.prompt([
      {
        type: 'input',
        name: 'name',
        message: 'Site name (you can change it later):',
-        default: siteSuggestion,
+        default: 'Pick for me',


I don't think we should be using the default property like this. We're assigning it a value that we know we won't use, just for the purpose of displaying something.

Instead, can we change message so that it reads something like Site name (leave blank for a random name; you can change it later), and we assign a value if the value is empty?

If we absolutely must use the current approach, let's please move the string to a constant.

I'll change it. I find the current approach a bit "hacky" too.

tinfoil-knight · 2022-09-05T13:25:16Z

Not sure as to what's happening with the failing tests here: https://github.com/netlify/cli/actions/runs/2993351770

danez · 2022-09-05T19:47:57Z

I'm not working till end of September. So i can look then or someone else does. Sorry

tinfoil-knight · 2022-09-30T11:47:51Z

@danez Can you take a look at the tests here now? They seem to be timing out. Still not sure what's causing this.

danez · 2022-10-06T15:31:11Z

@tinfoil-knight I rebased on the current main branch and fixed the tests. It seems the tests match on the exact input line.

outdated

danez

LGTM!

tinfoil-knight · 2022-10-06T15:48:09Z

@tinfoil-knight I rebased on the current main branch and fixed the tests. It seems the tests match on the exact input line.

Thank you a lot for fixing the tests @danez.

verythorough · 2022-10-07T23:36:37Z

This is awesome! Here I was going to post a list of the words we use to generate the site names in Netlify, and I see y'all settled on a better solution. ✨

tinfoil-knight commented Aug 28, 2022

View reviewed changes

src/utils/sites/utils.js Outdated Show resolved Hide resolved

tinfoil-knight marked this pull request as ready for review August 28, 2022 13:06

tinfoil-knight requested a review from danez August 28, 2022 13:06

tinfoil-knight self-assigned this Aug 28, 2022

tinfoil-knight added the type: bug code to address defects in shipped code label Aug 28, 2022

tinfoil-knight force-pushed the fix/site-naming branch from 3eb8386 to 03d545b Compare August 28, 2022 13:15

danez requested a review from a team August 29, 2022 12:14

tinfoil-knight marked this pull request as draft September 2, 2022 07:35

tinfoil-knight commented Sep 2, 2022

View reviewed changes

tinfoil-knight marked this pull request as ready for review September 2, 2022 10:07

eduardoboucas previously requested changes Sep 5, 2022

View reviewed changes

tinfoil-knight requested a review from eduardoboucas September 5, 2022 12:09

tinfoil-knight force-pushed the fix/site-naming branch from 9883cf2 to fd28bd0 Compare September 5, 2022 12:35

tinfoil-knight removed the request for review from danez September 5, 2022 20:03

tinfoil-knight requested a review from danez September 30, 2022 11:28

tinfoil-knight and others added 8 commits October 6, 2022 17:23

fix: follow standard pattern for auto-generated site names

9b73618

chore(deps): remove @sindresorhus/slugify as it is no longer needed

1e66442

docs: add comment to prevent people from adding invalid suggestion names

220559f

refactor: remove server dependent site name generation util

96146c0

fix: pass empty string if no name is provided

76c18d8

fix: use template name if site name is not available

9f0b699

fix: avoid using default value as display text

6db90f5

chore: fix tests

640b21d

danez force-pushed the fix/site-naming branch from d7b27d1 to 640b21d Compare October 6, 2022 15:30

danez approved these changes Oct 6, 2022

View reviewed changes

danez added type: bug code to address defects in shipped code and removed type: bug code to address defects in shipped code labels Oct 7, 2022

danez added the automerge Add to Kodiak auto merge queue label Oct 8, 2022

Merge branch 'main' into fix/site-naming

266e067

kodiakhq bot merged commit 7c5e675 into netlify:main Oct 8, 2022

token-generator-app bot mentioned this pull request Oct 8, 2022

chore(main): release 12.0.8 #5144

Merged

tinfoil-knight deleted the fix/site-naming branch October 8, 2022 10:46

CASABECI mentioned this pull request Oct 29, 2023

[Snyk] Upgrade fastify from 4.17.0 to 4.23.2 CASABECI/cli#4

Open

MaxMood96 mentioned this pull request Dec 1, 2023

[Snyk] Fix for 1 vulnerabilities MaxMood96/cli-2#156

Open

MaxMood96 mentioned this pull request May 14, 2024

[Snyk] Fix for 2 vulnerabilities MaxMood96/cli-2#163

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: follow standard pattern for auto-generated site names #5007

fix: follow standard pattern for auto-generated site names #5007

tinfoil-knight commented Aug 28, 2022 •

edited

Loading

github-actions bot commented Aug 28, 2022 •

edited

Loading

tinfoil-knight commented Aug 28, 2022 •

edited

Loading

danez commented Aug 29, 2022

tinfoil-knight commented Aug 29, 2022 •

edited

Loading

danez commented Aug 29, 2022 •

edited

Loading

whitep4nth3r commented Aug 30, 2022

tinfoil-knight commented Aug 30, 2022 •

edited

Loading

whitep4nth3r commented Aug 30, 2022

tinfoil-knight commented Aug 30, 2022

whitep4nth3r commented Aug 30, 2022

tinfoil-knight commented Aug 30, 2022 •

edited

Loading

whitep4nth3r commented Aug 30, 2022

tinfoil-knight commented Aug 30, 2022

tinfoil-knight commented Sep 2, 2022 •

edited

Loading

tinfoil-knight left a comment

eduardoboucas Sep 5, 2022

tinfoil-knight Sep 5, 2022

tinfoil-knight commented Sep 5, 2022

danez commented Sep 5, 2022

tinfoil-knight commented Sep 30, 2022 •

edited

Loading

danez commented Oct 6, 2022

danez left a comment

tinfoil-knight commented Oct 6, 2022

verythorough commented Oct 7, 2022

fix: follow standard pattern for auto-generated site names #5007

fix: follow standard pattern for auto-generated site names #5007

Conversation

tinfoil-knight commented Aug 28, 2022 • edited Loading

Summary

github-actions bot commented Aug 28, 2022 • edited Loading

📊 Benchmark results

Package size: 227 MB

tinfoil-knight commented Aug 28, 2022 • edited Loading

danez commented Aug 29, 2022

tinfoil-knight commented Aug 29, 2022 • edited Loading

danez commented Aug 29, 2022 • edited Loading

whitep4nth3r commented Aug 30, 2022

tinfoil-knight commented Aug 30, 2022 • edited Loading

whitep4nth3r commented Aug 30, 2022

tinfoil-knight commented Aug 30, 2022

whitep4nth3r commented Aug 30, 2022

tinfoil-knight commented Aug 30, 2022 • edited Loading

whitep4nth3r commented Aug 30, 2022

tinfoil-knight commented Aug 30, 2022

tinfoil-knight commented Sep 2, 2022 • edited Loading

tinfoil-knight left a comment

Choose a reason for hiding this comment

eduardoboucas Sep 5, 2022

Choose a reason for hiding this comment

tinfoil-knight Sep 5, 2022

Choose a reason for hiding this comment

tinfoil-knight commented Sep 5, 2022

danez commented Sep 5, 2022

tinfoil-knight commented Sep 30, 2022 • edited Loading

danez commented Oct 6, 2022

danez left a comment

Choose a reason for hiding this comment

tinfoil-knight commented Oct 6, 2022

verythorough commented Oct 7, 2022

tinfoil-knight commented Aug 28, 2022 •

edited

Loading

github-actions bot commented Aug 28, 2022 •

edited

Loading

tinfoil-knight commented Aug 28, 2022 •

edited

Loading

tinfoil-knight commented Aug 29, 2022 •

edited

Loading

danez commented Aug 29, 2022 •

edited

Loading

tinfoil-knight commented Aug 30, 2022 •

edited

Loading

tinfoil-knight commented Aug 30, 2022 •

edited

Loading

tinfoil-knight commented Sep 2, 2022 •

edited

Loading

tinfoil-knight commented Sep 30, 2022 •

edited

Loading