Improve support for underscores in field names

[salomoneb]

Description

This PR:

1. Fixes an issue where schema fields prefixed with more than one underscore are recognized as invalid.
2. Renames isCamelCase() to more accurately describe the function's intended purpose, which is to check for the presence of invalid characters in property names.

Re 1): our team has a code convention where certain properties begin with double underscores. When components using these properties are rendered, we get something like the following:

This is because isCamelCase() only accounted for single underscores.

Re: 2), I realized that isCamelCase() was not actually checking that a key was camelCase, but instead verifying that it didn't have any characters that would be invalid for Kiln's dotNotation() function (which allows numbers for some reason, even though these aren't valid in dot notation). I renamed isCamelCase to isValidKilnDotNotation to reflect this.

Testing

To test, add a schema field beginning with multiple underscores to a component and use that field in your template. Confirm that there are no errors. Here's an example of how we're actually using a double underscored field, __isUniqueInstance, in our app:

strapline:
  _reveal:
    field: __isUniqueInstance
    operator: falsy
  _has:
    input: text
    validate:
      required:
        field: straplineUrl
        operator: truthy

__isUniqueInstance:
  _has:
    help: |
      Indicates whether this is a unique instance e.g. for jazzy cover stories

_groups:
  notableLinksInfo:
    _label: Notable Links
    _placeholder:
      ifEmpty: notableLinks
    fields:
      - notableLinks
      - __isUniqueInstance #put here cause _label only works with 2+ fields grouped -- todo test this is still true

[james-owen]

If we just want to account for this __ case, what do you think of just adding it to this expression?

Suggested change

	return str.toLowerCase() === camel.toLowerCase() || str.toLowerCase() === `_${camel}`.toLowerCase() || str.toLowerCase() === `__${camel}`.toLowerCase();

[james-owen]

Also, we'll need to update the corresponding function in claycli: https://github.com/clay/claycli/blob/master/lib/cmd/lint.js#L272

[salomoneb]

Maybe I'm misunderstanding, but that regex should account for an arbitrary number of underscores, no? Is there a scenario I'm missing?

I can put in a PR for ClayCli. There's no place common to both of these repos where we could put this function, is there?

[james-owen]

Yep. Do you think it's necessary to support more than two underscores? If you just need to add that one case, what do you think of just adding that one __ case?

[salomoneb]

There were a few things I was thinking about here:

1. In this particular scenario, we are only supporting two underscores, but what if someone wanted three? Is there a reason we wouldn't want to support an arbitrary number of leading underscores?

2. 1 sort of gets into the bigger issue, which is that the function was supposedly created to check that a string was camel case, but the description inside of it said that it was there to prevent people from "using weird characters that cannot be used as properties in dot notation". And that's what it did! Even though it was called isCamelCase, it didn't actually check that a string was camel cased; it used Lodash's _.camelCase as a validator and then compared the lowercased result to the lowercased original string. The purpose of this was to make sure that Schema field names are valid dot notation property accessors (even though, as I note in my comment, numbers aren't valid).

I can add the double underscore as an exception and it'll work, but this seems like a great opportunity to try to improve a misleading piece of code (and remove an unnecessary use of Lodash). There are many parts of Clay that are very confusing because they have lots of complex and/or procedural steps, and it's always nice when we have the chance to simplify things.

[reubenson]

can't say i have enough history as a core clay contributor to weigh in definitively, but my sense is that a potential pattern incorporating double underscore props (to indicate an internal, non-user-editable field?) is somewhat at odds with Clay's general pattern of restricting underscore to indicate reserved properties.

i don't think allowing double underscores would break anything, but I'm more concerned that allowing users to arbitrarily name properties with a single underscore would dilute the pattern to reserve such properties under the strict domain of the CMS (and not the developer)

that said, the pattern of double-underscore properties isn't exactly esoteric either, and i don't think this PR should necessarily be blocked from merging. and i do agree that the naming of the isCamelCase function is misleading, since it coercively strips out all underscores

[salomoneb]

In our case, the double underscore represents a computed property (some value that we've generated on render and use in the template, but don't actually want to save to the database).

I understand about not wanting users to override the framework's single underscore pattern. However (and correct me if I'm wrong), I don't think this PR changes that. Users could - and can still - create field names with single underscores if they chose; isCamelCase never prevented people from doing that. In fact, we adopted the double underscore convention so that we wouldn't hijack those reserved properties.

I should say that one of the other reasons why I made this change is that the scope seems relatively small; isCamelCase was never exported and was only used as a helper function for assertCamelCasedProps (as well as in clay-cli, as James pointed out).

[reubenson]

@salomoneb ah, thanks for clearing that up! given that, these changes seem quite sensible

[salomoneb]

🎉 Thank you!

[salomoneb]

Oh, I'll put in a PR on the clay-cli side. What's the best way to fix the failing CI test?

[james-owen]

@salomoneb I think the "tests failed" message is ok for now. Just an issue with the pipeline, not actually a failing test.

[james-owen]

@salomoneb would you mind rebasing/merging the latest master? Then I'll get this merged in.

[mattoberle]

@salomoneb this was released in clay-kiln@8.18.0.

Please note the corresponding claycli upgrade you'll want to pair with this feature.