`calc-macos` has no description tab

arabello commented 1 month ago

The next-mdx-remote serialization of the package's README fails because the source markdown misses a closing <img> tag. This causes the getStaticProps routine to fail and fallback to an empty (None) package rendering nothing.

A solution addressing the serialization failure should be investigated if we want to support the missing closing tag syntax
The fastest mitigation is to update the original source README, but this involves the hub
We could think about to exclude the package entirely from the build if a failure is raised at this level

smeech commented 1 month ago

I have access on the hub if something needs changing on the calc-macos package, if that helps.

smeech commented 1 month ago

If the line:

<img width="185" alt="image" src="https://user-images.githubusercontent.com/23709916/175305102-2453d39b-b7d8-45f2-8a42-9286a2ab2d25.png">

just needs a </img> on the end I can edit that in place.

I agree - it would be better if a failure of this sort prevented the merge.

arabello commented 1 month ago

Yes, it should just need the closing tag or the auto-closing one (/>). As a support, here the stack trace

{
  _tag: 'Left',
  left: Error: calc-macos-0.1.0: Failing to serialize readme markdown: SyntaxError: unknown: Expected corresponding JSX closing tag for <img>. (16:0)

    14 | <li parentName="ol">{`You'll see a form, type the calculation`}
    15 | <img width="185" alt="image" src="https://user-images.githubusercontent.com/23709916/175305102-2453d39b-b7d8-45f2-8a42-9286a2ab2d25.png">
  > 16 | </li>
       | ^
    17 | <li parentName="ol">{`The text will be replaced with the result`}</li>
    18 | </ol>
    19 | <h2>{`Commands`}</h2>
     ....
}

If you can patch it on the hub it would be grate, because if we are like we also solve https://github.com/espanso/hub/issues/114 and the new build should trigger a re-index.

I agree - it would be better if a failure of this sort prevented the merge.

Absolutely, I'll track it on another issue though

smeech commented 1 month ago

Done.

arabello commented 1 month ago

Unfortunately, the package and the index were not updated: the logic checking for delta changes between the released and the repository packages does not take in account the single package change. The package requires a new version to be updated, otherwise we need to manually update the artifacts.

Sorry for the confusion

smeech commented 1 month ago

So if I submit a PR for the package, unchanged except for the version that should fix it? It's something I had considered trying, although without the </img> amendment it would have failed. I'll get onto it.

smeech commented 1 month ago

Success!

Thank you!

AucaCoyan commented 1 month ago

Great! It is my understanding that the package calc-macos is resolved, but we don't have a solution to prevent it in the future, right?

arabello commented 1 month ago

we don't have a solution to prevent it in the future, right?

Correct, I opened #44 which seems to me the general approach we should take but let's discuss it. For the specific serialization issue instead we can investigate farther, but we depend on the third party lib logic

The package https://hub.espanso.org/calc-macos still has the empty v.0.1.0 but it should be solved by #44

The current version instead v0.1.1 is missing the description tab (README) even if the README string is correctly fetched. I'll update the title to track this issue

arabello commented 1 month ago

I find out that the issue is the lack of homepage property inside the calc-macos' manifest. If the property is missing, the hub-frontend discards the entire description tab.

The homepage URL is used to correctly renders the resources assets from Github. I might remember wrong, but I think it was required as the Github user's README may contain relative paths to assets (img, links, ...). As Github resolves those paths, we do the same to point to the Github hosted assets.

At the same time, the Package Specification states that the homepage property is optional and may points to something else rather than Github repository.

I think the hub-frontend feature was designed without taking in account this policy along with a lack of communication between the hub and hub-frontend requirements.

It would not be fair to make it mandatory if the hub does not actually require it to work properly. At the same time, the hub-frontend relies on it.

IMHO the issue is non-blocking as the package is anyway shown in the hub-frontend: I would not considered it critical or a bug, rather an improvement. I would like an opinion from other maintainers though.

I will be back if I find a better way to handle those assets, without giving up the feature and the property optionality within the hub

AucaCoyan commented 1 month ago

Nice findings! I think you found the solution already. I'm on the side of making a property required, if some app is expecting it. It's a bit more tough on the users to fill one property, but it also gains of better documentation in the long run. You are the original code designer, so I will take everything you prefer as a solution 🙌

smeech commented 1 month ago

The lack of a homepage: doesn't appear to prevent successful expression in the hub website - https://github.com/espanso/hub/blob/main/packages/divination-oracles/0.1.0/_manifest.yml comes to mind as a recently successful merge.

I have no qualms about making it obligatory, however, if it's needed, but I don't quite understand whether it is or not. I doubt if many users would make use of it, especially if it is just a GitHub repo, and those who really want access to the latter can generally find it via the commit.

I already insist on tags: because they improve the user-experience on the hub-website by making it easier to find packages of interest, and, in the interest of quality, I occasionally make suggestions as to improvements in the code.

Once we've decided, I'll make the necessary amendments to Package Specification.

b02860de585071a2 commented 1 month ago

Just to make sure I understand this correctly, this the same reason why some packages load an entirely blank page, correct? Don't want to open a duplicate.

Examples from the search page: https://hub.espanso.org/espanso-pinyin-tones https://hub.espanso.org/vim-digraphs

AucaCoyan commented 1 month ago

@b02860de585071a2 correct!

smeech commented 1 month ago

Just to make sure I understand this correctly, this the same reason why some packages load an entirely blank page, correct? Don't want to open a duplicate.

Examples from the search page: https://hub.espanso.org/espanso-pinyin-tones https://hub.espanso.org/vim-digraphs

We need to drill down into the particular reasons why certain packages don't get properly merged in Espanso Hub. I thought there might be more! In calc-macos case, the README.md was missing an </img> tag, but others probably have slightly different reasons.

I'll try and have a look, but may need @arabello's help as I don't know where the logs of the process are stored. Do let us know if you find any more, as we'll need to look at them individually.

b02860de585071a2 commented 1 month ago

Do let us know if you find any more, as we'll need to look at them individually.

All the others I'm aware of:

arabello commented 1 month ago

Just to make sure I understand this correctly, this the same reason why some packages load an entirely blank page, correct?

A blank page is shown when the resolution of packageRepo data model fails. The packageRepo is resolved by getStaticProps: there can be multiple failure reasons. For the calc-macos case, the reason was the serializeReadme sub routine caused by unsupported README syntax of the third party library we use (next-mdx-remote/serialize). For the others packages listed, the unsupported syntax is probably the issue but we can't say for sure without looking into it.

Instead, the missing description tab, this current issue focus, is related to the lack of homepage property in the _manifest.yml.

I don't know where the logs of the process are stored

Unfortunately, we don't have logs. Every time the hub-frontend CI publishes a new version we can look at the Next.js build logs. However, this kind of issues are silent: by using func. paradigm via fp-ts we (quite) never hard fails, but we fallback to default behaviours. Here we have two induced issues:

the default behaviour is not user friendly: having a blank page is ineffective and it was a bad choice (my bad, srry guys)
we should add info logs so when a package fails to be resolved at build time we can easily look into it.

I'm currently having busy days, but I'll try to be more operative. I would still suggest to breakdown all the issues to better prioritize and divide work load. Something like:

homepage property (this issue) > define the strategy accordingly to hub manifest requirements and eventually implement necessary code in hub-frontend
investigate how to better support README syntax by Github so to avoid the issues by design. For example, I remember I straightforward string replacement which can be helpful. I would prefer to tackle this issue in a more system-wide approach, ie. finding a better third party library or configure the current one to support everything (or quite) that Github markdown resolver does.
- Replace the blank page with something at least more useful for the user
- Add build-time logs for debugging and control purposes

smeech commented 1 month ago

Thank you for commenting, and I'm sorry we're leaning on you for this - you seem to be the only person who understands what's going on!

For the time being, I can defer merging for any packages that don't have a homepage:, or add the tag without a value if it's unavailable, if that would work.

smeech commented 2 weeks ago

One I have just merged has a blank page :cry: https://github.com/espanso/hub/tree/main/packages/discord-snippet/0.1.0

espanso / hub-frontend

`calc-macos` has no description tab #43