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

use ember-showdown-shiki #902

Merged
merged 7 commits into from
Jul 8, 2024
Merged

use ember-showdown-shiki #902

merged 7 commits into from
Jul 8, 2024

Conversation

mansona
Copy link
Member

@mansona mansona commented Jan 12, 2024

This makes use of the changes in ember-learn/ember-jsonapi-docs#137 and ember-learn/ember-api-docs-data#25 to make sure that we're using markdown/showdown for our descriptions everywhere now.

This has allowed us to use https://github.com/IgnaceMaes/ember-showdown-shiki which (finally) gives us the same functionality for syntax highlighting across guides and API docs 🎉

Copy link

netlify bot commented Jan 12, 2024

Deploy Preview for ember-api-docs ready!

Name Link
🔨 Latest commit cf5daf8
🔍 Latest deploy log https://app.netlify.com/sites/ember-api-docs/deploys/66880a832bd0db00089facda
😎 Deploy Preview https://deploy-preview-902--ember-api-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mansona mansona force-pushed the markdown branch 4 times, most recently from 1953f71 to 8b2f688 Compare June 25, 2024 12:57
@mansona mansona force-pushed the markdown branch 2 times, most recently from 6212aab to 52c799f Compare July 5, 2024 12:24
@mansona mansona changed the title use ember-showdown-prism use ember-showdown-shiki Jul 5, 2024
{{#if this.showClipboardSuccessIcon}}
{{svg-jar 'success' width='24px' height='24px'}}
{{else}}
<CopyButton @clipboardText={{concat 'import ' @item " from '" @package "';"}} @title='Copy to clipboard' @success={{this.showSuccess}}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: interesting, wasn't aware we had this functionality in the API docs. Could be something to build into ember-showdown-shiki 😄

@IgnaceMaes
Copy link
Member

suggestion: configure the languages to load in config/environment.js. This way we only load the required language grammars.

See e.g. https://github.com/ember-learn/cli-guides/pull/316/files#diff-9764cd8b6365b92786ac2e7eecab5e31a51193664db8931af01778ab9d0fa76c

    // ...
    'ember-showdown-shiki': {
      theme: 'github-dark',
      languages: [
        'bash',
        'css',
        'http',
        'javascript',
        'json',
        'json5',
        'ruby',
        'scss',
        'yaml',
        'typescript',
        'glimmer-js',
        'glimmer-ts',
        'handlebars',
      ],
    },

@IgnaceMaes
Copy link
Member

Did some spot checks, and it looks all good to me 🙌

@mansona
Copy link
Member Author

mansona commented Jul 6, 2024

suggestion: configure the languages to load in config/environment.js. This way we only load the required language grammars.

what is the alternative? does it load all of them?

@IgnaceMaes
Copy link
Member

what is the alternative? does it load all of them?

That's the default behaviour, yes.

@mansona
Copy link
Member Author

mansona commented Jul 8, 2024

in that case I wouldn't consider this a blocker, we can fix it as an optimisation later. I wouldn't be comfortable limiting the languages on the api-docs unless we had some sort of lint checking that the input Markdowns are only using the right languages, does that make sense?

@IgnaceMaes
Copy link
Member

The language grammars are loaded eagerly, so loading all of them does have a performance impact.

I think it should be safe taking over the ones we set for the other guide apps. And if a language is missing, it would just be not highlighted (but not crash).

The alternative is looking into making loading the grammars lazy in ember-showdown-shiki. Not sure how feasible that is 🤔

@mansona
Copy link
Member Author

mansona commented Jul 8, 2024

So a few things here:

I'm not exactly sure it doesn't crash if the language doesn't exist. I had to do a process in ember-learn/ember-jsonapi-docs#137 that stripped the language no-highlight from the code blocks because the app would crash. I don't remember if this was on the branch before or after I switched from ember-showdown-prism to ember-showdown-shiki so my data could be out of date. If you wanted to verify that it's ok to highlight with a missing language (including in prember) then I'm happy to limit it like you say 👍

As for dynamic loading of languages, I'm 100% against this. We need it to be predictable and I don't want to open up an issue of executing random JS modules from a remote location in a fastboot environment. I would rather the language be missing (and not crash) than loading it dynamically 👍

Copy link
Contributor

@MinThaMie MinThaMie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LETS GOOOOOO 🚢

@MinThaMie MinThaMie merged commit 46a7bdf into main Jul 8, 2024
4 checks passed
@MinThaMie MinThaMie deleted the markdown branch July 8, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants