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

calc-macos has no description tab #43

Open
arabello opened this issue Jun 28, 2024 · 21 comments
Open

calc-macos has no description tab #43

arabello opened this issue Jun 28, 2024 · 21 comments
Assignees
Labels
bug Something isn't working

Comments

@arabello
Copy link
Collaborator

arabello commented Jun 28, 2024

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
@arabello arabello added the bug Something isn't working label Jun 28, 2024
@arabello arabello self-assigned this Jun 28, 2024
@smeech
Copy link

smeech commented Jun 30, 2024

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

@smeech
Copy link

smeech commented Jun 30, 2024

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
Copy link
Collaborator Author

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 espanso/hub#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
Copy link

smeech commented Jun 30, 2024

Done.

@arabello
Copy link
Collaborator Author

arabello commented Jun 30, 2024

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
Copy link

smeech commented Jun 30, 2024

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
Copy link

smeech commented Jun 30, 2024

Success!

Thank you!

@AucaCoyan
Copy link
Member

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
Copy link
Collaborator Author

arabello commented Jul 1, 2024

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 arabello changed the title calc-macos has an empty page calc-macos has no description tab Jul 1, 2024
@arabello
Copy link
Collaborator Author

arabello commented Jul 4, 2024

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
Copy link
Member

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
Copy link

smeech commented Jul 4, 2024

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
Copy link

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
Copy link
Member

@b02860de585071a2 correct!

@smeech
Copy link

smeech commented Jul 8, 2024

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.

@arabello
Copy link
Collaborator Author

arabello commented Jul 9, 2024

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
Copy link

smeech commented Jul 9, 2024

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
Copy link

smeech commented Aug 6, 2024

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

@AucaCoyan
Copy link
Member

Is this still a problem? do we accept packages without description in the hub? I think we do before, so if it's okay with you, we can close this as completed.
@smeech what do you think?

@smeech
Copy link

smeech commented Oct 8, 2024

It's still a problem.

html-colors was a recent merge where the Description tab failed, despite a good README.md which the author and I had discussed and amended prior to merging.

espanso/hub#130

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants