Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decode Bowerfiles as lenient JSON and only parse necessary fields #211

Closed
Tracked by #216
thomashoneyman opened this issue Sep 1, 2021 · 1 comment · Fixed by #221
Closed
Tracked by #216

Decode Bowerfiles as lenient JSON and only parse necessary fields #211

thomashoneyman opened this issue Sep 1, 2021 · 1 comment · Fixed by #221

Comments

@thomashoneyman
Copy link
Member

thomashoneyman commented Sep 1, 2021

In #210 we saw a number of packages fail with a MalformedBowerJson error, namely because of issues like this:

{
  "name": "purescript-facebook",
  "description": "Idiomatic Purescript bindings for the Facebook SDK",
  "license": "Apache-2.0",
  "keywords": "facebook purescipt",
  ...
}

This fails to decode as a Bower.PackageMeta because the keywords field has to be an array of keywords, not a plain string. We don't even use this field in our manifests, though, so we shouldn't care if it isn't Bower-compliant!

The errors aren't always just that the file can't be decoded as a Bower.PackageMeta. Sometimes they aren't valid JSON at all:

{
  "name": "purescript-endpoints-express",
  "version": "0.0.1",
  "authors": {
    "Simon Van Casteren <simon.van.casteren@gmail.com>"
  },
  ...
}

This file isn't valid JSON because of the authors key, which has an object with no key/value pairs as its value. We can't parse this as Json at all.

And yet we can pull off the keys that we know need to be there -- the name, the version, the dependencies, and the devDependencies -- and attempt to decode their contents. To avoid needlessly removing packages from the registry, I suggest that we change how we parse Bowerfiles once #210 merges so that packages like these can be converted into manifests despite their JSON issues.

There are some we maybe still can't parse as Json, because even the keys we're looking for have little issues. For example, the next one is even more insidious:

{
    "name": "purescript-uint",
    ...
    "dependencies": {
        "purescript-enums": "^v5.0.0",
        "purescript-gen": "^v3.0.0",
        "purescript-math": "^v3.0.0",
        "purescript-maybe": "^v5.0.0",
        "purescript-prelude": "^v5.0.0",
        "purescript-psci-support": "^v5.0.0",
    }
    "devDependencies": {
        "purescript-effect": "^v3.0.0",
        "purescript-quickcheck": "^v7.1.0",
        "purescript-quickcheck-laws": "^v6.0.1"
    }
}

There's a trailing comma after the "purescript-psci-support" dependency! It's not valid JSON! It would be nice if we could handle this case too, but perhaps we just pull off the keys we need and if they're not valid JSON then we just drop the package.

@f-f
Copy link
Member

f-f commented Sep 8, 2021

A summary for this issue - this is really two parts:

  • our current JSON parser is strict, and that prevents us from using "malformed" JSON (with trailing commas, and so on). We should use a "more lenient" JSON parser, something like this one so that we could decode more JSON files
  • we are currently decoding the JSON into a Bower.PackageMeta - this conversion is also strict and fails even when documents have excess keys. We instead want to use these documents, so we should make our own Bowerfile type where we only decode things that we need

@thomashoneyman thomashoneyman changed the title Don't decode Bowerfiles as JSON when converting to manifests Decode Bowerfiles as lenient JSON and only parse necessary fields Sep 8, 2021
@thomashoneyman thomashoneyman linked a pull request Sep 10, 2021 that will close this issue
@f-f f-f closed this as completed in #221 Sep 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants