Validate name upon init fixes #1268

[olingern]

Summary
I made use of the existing package name validation and put this inside a loop to enforce valid names, i.e. no spaces.
Fixes #1268

Test plan
Tests for validating user input will need to be added. Guidance, best practices, advice is welcome.

[olingern]

I should also note that eslint is pretty unhappy about aliasing a function for later use in src/cli/commands/

Name configuration:

{
  key: 'name',
  question: 'name',
  default: path.basename(config.cwd),
  validation: validate.isValidPackageName,
  validationError: 'Invalid package name',
}

while (!validAnswer) {
  answer = (await reporter.question(question)) || def;
  /* eslint-disable  no-var*/
  entry.validation(answer) ? validAnswer = true : reporter.error(entry.validationError);
   /* eslint-disable */
}

I'm unsure why eslint doesn't seem to be able to ignore this line.

As for other approaches, direct / manual validation on the name could be an option, but attaching validation properties to each entry offers a bit more extensibility.

[torifat]

Yarn uses flow. So, beside checking with eslint also check your code with flow. Just run yarn run lint. BTW, you might want you provide a more meaningful feedback to user as npm does.

Sorry, name can only contain URL-friendly characters and name can no longer contain capital letters.

[torifat]

This will make your code pass the lint:

    let answer;

    if (yes) {
      answer = def;
    } else {
      let validAnswer = false;
      // loop until a valid answer is provided, if validation is on entry
      if (entry.validation) {
        while (!validAnswer) {
          answer = (await reporter.question(question)) || def;
          answer && entry.validation(answer) ? validAnswer = true : reporter.error(entry.validationError);
        }
      } else {
        answer = (await reporter.question(question)) || def;
      }
    }

[olingern]

@torifat One of the problems is that I've added a property to the keys that builds the question loop. This property, validation, holds a function that is later executed. Flow doesn't like it because it has no idea what entry.validation( ) is.

src/cli/commands/init.js:116
116:           entry.validation(answer) ? validAnswer = true : reporter.error(entry.validationError);
                      ^^^^^^^ property `validation`. Property not found in
116:           entry.validation(answer) ? validAnswer = true : reporter.error(entry.validationError);
                      ^^^^^ object literal

src/cli/commands/init.js:116
116:           entry.validation(answer) ? validAnswer = true : reporter.error(entry.validationError);
                                           ^^^^^^ possibly uninitialized variable. This type is incompatible with
 41: export function isValidPackageName(name: string): boolean {
                                         ^^^^^^ string. See: src/util/normalize-manifest/validate.js:41

src/cli/commands/init.js:116
116:           entry.validation(answer) ? validAnswer = true : reporter.error(entry.validationError);
                                                                                    ^^^^^^^^^^^^^^^ property `validationError`. Property not found in
116:           entry.validation(answer) ? validAnswer = true : reporter.error(entry.validationError);
                                                                              ^^^^^ object literal

[torifat]

@olingern did you try replacing your code with the snippet I provided? Use if (entry.validation) { instead of if (entry.hasOwnProperty) {. Flow doesn't understand hasOwnProperty -> facebook/flow#2008

[olingern]

@torifat Thanks! I overlooked this

if (entry.validation)

Your comment "Flow doesn't understand hasOwnPropert" helped a bunch!

[olingern]

@samccone Just made a few updates to tidy things up.

• Validation uses the pre-existing language file to generate the error message
• Condensed git history

[torifat]

@samccone Invalid package name. didn't seem like a user friendly message. npm uses the following:

Sorry, name can only contain URL-friendly characters and name can no longer contain capital letters.

[olingern]

@torifat @samccone I can update the invalidPackageName key in src/reporters/lang/en.js

I assume I would translate the validateName function in validate.js into something meaningful such as:

Invalid package name. Spaces and the following symbols: " @ + % : " are not allowed

isValidName function for reference

function isValidName(name) {
  return !name.match(/[\/@\s\+%:]/) && encodeURIComponent(name) === name;
}

[sebmck]

We're hardcoding the error message here which doesn't seem desirable, can you move this to a validationError property on keys?

[olingern]

@kittens I opened a new PR #1425. Issues with my local env. Your suggest changes are in there as well as a cleaner git history. :)

[sebmck]

Can you add a space after // to pad this out? Just to keep consistency with how we do comments elsewhere. eg:

// loop

rather than

//loop