Registry package name limitations are also applied to local packages

[finnhodgkin]

Legacy spago didn't have length validation on local package names, whereas spago@next has a strict 50-character limit.

Unfortunately in the project I'm migrating we have a bunch of generated libraries with really long names like oa-gql-enums-author-field-response-versions-select-column. It wouldn't be the end of the world if we had to cut the names down or merge the generated libraries, but it feels like an arbitrary limit for unpublished local packages.

Seems to be caused by the spago.yaml being read using the parser from registry-dev:

Spago config parser

Registry name parser

Other than this the migration was going great, I'm really loving the new workspace features! 🎉

Minimal example:

package:
  dependencies:
    - console
    - prelude
  name: a-really-long-package-name-that-is-53-characters-long
  test:
    dependencies: []
    main: Test.Main
workspace:
  extra_packages: {}
  package_set:
    registry: 43.0.0

Results in:

Reading Spago workspace configuration...

❌ Couldn't parse Spago config, error:
An error occurred while decoding a JSON value:
Under 'Config':
At object key package:
Under 'PackageConfig':
At object key name:
Under 'PackageName':
Unexpected value "a-really-long-package-name-that-is-53-characters-long".
Run spago init to initialise a new project.

[finnhodgkin]

Happy to make a PR some time this week if it's an unintentional change. The 50 character limit for published packages feels legit but I could break the parser into local and remote versions.

[f-f]

Thanks for reporting! I had no suspicion whatsoever that this would ever be an issue, and yet 😄

I am a bit hesitant to split the types that Spago uses from the Registry ones, especially the PackageName. I think we picked the 50-char limit somewhat arbitrarily, so I guess moving the post would not be a big deal - what do you think @thomashoneyman?

[f-f]

@finnhodgkin what's the longest package name you have?

[finnhodgkin]

150 characters 🙈

[thomashoneyman]

I think the limit was chosen arbitrarily and could be raised. But it seems to me the more correct thing to do would be to allow local package names to be arbitrarily long.

Elsewhere in the registry we use wrapper types like LenientVersion to reuse the underlying parser for the Version type but apply some pre-processing:
https://github.com/purescript/registry-dev/blob/master/app/src/App/Legacy/LenientVersion.purs

Perhaps you could take a similar approach in Spago. We could update the package name parser in the registry to return a structured error:

data PackageNameError = TooLong Int | InvalidChar String | ...

printPackageNameError :: PackageNameError -> String

parse :: String -> Either PackageNameError PackageName

Then, we can do two things. First, we can return a better parse error in the registry when a name is too long (instead of just "unexpected value"). Second, Spago can create a new LenientPackageName for local packages that parses the package name using the Registry parser and, on error, if it's the "Too Long" error, it can still accept the name.

This keeps Spago using the parser from the registry in general but allows some leniency for local packages, which don't have to have the same restrictions applied in the registry.

Just a thought. I don't want to start relaxing restrictions on the registry types to satisfy quirks in local packages if we can help it.

[f-f]

But it seems to me the more correct thing to do would be to allow local package names to be arbitrarily long.

I don't think that should be allowed - there's not much point to a 4KB long package name.
So, given that we'd limit the length on local packages anyways, this might just be about finding a threshold that we feel comfortable in both Spago and the Registry. Maybe 128 chars works?

The reason why I'm not eager to split off the PackageName type in Spago is that it's used everywhere and we interop with the Registry datatypes all the time, and would be very annoying to convert back and forth in all these places, all while they have the same parsing rules and the only difference being that one allows 50 and the other 128 chars

[thomashoneyman]

I don't really know what the limit should be — crates.io uses 64 characters, as one data point — I just know we need some limit. We can change it to 128 characters, that's fine with me.

[f-f]

And npm uses 214 chars (funnily enough they started with 50 as well).

Let's do 128 and pour some concrete in there 😄

[JordanMartinez]

Why not do 150 characters? It's not much more and doesn't cause any more work for the @finnhodgkin above.

[f-f]

Sure, that works too!

[finnhodgkin]

Either works for us, thanks! Bit cheeky of me to pop up and say 'we've got some funky codegen that produces ridiculously long names, can spago be the one to change?'.

Really the only part that was a pain was working out where the issue actually came from. The parser error is ignored by spago, plus comes from a different repo so searching '50' or the error message doesn't work.

Unexpected value "a-really-long-package-name-that-is-53-characters-long".
vs
Package name cannot be longer than 50 characters

Also because the json parser is second in the <|> for dependency parsing, it's even more confusing:

❌ Couldn't parse Spago config, error:
An error occurred while decoding a JSON value:
Under 'Config':
At object key package:
Under 'PackageConfig':
At object key dependencies:
At array index 2:
Expected value of type 'Object'.
Run spago init to initialise a new project.

[f-f]

@finnhodgkin ah yes that makes sense. I think our codecs could do with some improvement and better error reporting (see #1057 as well)