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

feat: better markdown support #5366

Closed
wants to merge 10 commits into from
Closed

Conversation

Jarsen136
Copy link
Contributor

Thank you for your contribution to the KodaDot NFT gallery.

👇 _ Let's make a quick check before the contribution.

PR Type

  • Bugfix
  • Feature
  • Refactoring

Context

image

Before submitting pull request, please make sure:

  • My contribution builds clean without any errors or warnings
  • I've merged recent default branch -- main and I've no conflicts
  • I've tried to respect high code quality standards
  • I've didn't break any original functionality
  • I've posted a screenshot of demonstrated change in this PR

Optional

  • I've tested it at </bsx/collection>
  • I've tested PR on mobile
  • I've written unit tests 🧪
  • I've found edge cases

Had issue bounty label?

  • Fill up your KSM address: Payout

Community participation

Screenshot 📸

  • My fix has changed something on UI; a screenshot is best to understand changes for others.

image

image

`/rmrk2/gallery/13898634-b46a62619cb12c7a15-LUM-A-G-0443_514_MB_GREGORY_THE_ILLUMINATOR-00000443`

@Jarsen136 Jarsen136 requested a review from a team as a code owner March 25, 2023 08:04
@Jarsen136 Jarsen136 requested review from preschian and vikiival and removed request for a team March 25, 2023 08:04
@kodabot
Copy link
Collaborator

kodabot commented Mar 25, 2023

SUCCESS @Jarsen136 PR for issue #5361 which is assigned to you. Please wait for review and don't hesitate to grab another issue in the meantime!

@netlify
Copy link

netlify bot commented Mar 25, 2023

Deploy Preview for koda-nuxt ready!

Name Link
🔨 Latest commit 9b6dc63
🔍 Latest deploy log https://app.netlify.com/sites/koda-nuxt/deploys/6429e4f2e7585f00083d938b
😎 Deploy Preview https://deploy-preview-5366--koda-nuxt.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 settings.

@socket-security
Copy link

socket-security bot commented Mar 25, 2023

New dependency changes detected. Learn more about Socket for GitHub ↗︎


🚨 Potential security issues found in this pull request. To accept the risk, merge this PR and you will not be notified again.

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] bar@* or ignore all packages with @SocketSecurity ignore-all

📜 Install scripts

Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts.

Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead.

Package Script field Source
@sentry/[email protected] (upgraded) install pnpm-lock.yaml via @sentry/[email protected]
[email protected] (upgraded) postinstall package.json, pnpm-lock.yaml via
[email protected] (upgraded) postinstall pnpm-lock.yaml via @kodadot1/[email protected], [email protected], libs/ui/package.json via [email protected]
@fortawesome/[email protected] (upgraded) postinstall pnpm-lock.yaml via @fortawesome/[email protected], @fortawesome/[email protected], @fortawesome/[email protected], @fortawesome/[email protected]
@fortawesome/[email protected] (upgraded) postinstall package.json, pnpm-lock.yaml via
@fortawesome/[email protected] (upgraded) postinstall package.json, pnpm-lock.yaml
@fortawesome/[email protected] (upgraded) postinstall package.json, pnpm-lock.yaml
@fortawesome/[email protected] (upgraded) postinstall package.json, pnpm-lock.yaml
[email protected] (upgraded) postinstall pnpm-lock.yaml via , libs/static/package.json via @vitest/[email protected], [email protected], libs/ui/package.json via [email protected]
🍣 Git dependency

Contains a dependency which resolves to a remote git URL. Dependencies fetched from git URLs are not immutable can be used to inject untrusted code or reduce the likelihood of a reproducible install.

Publish the git dependency to npm or a private package repository and consume it from there.

Package Dependency Location Source
[email protected] (added) @iktakahiro/markdown-it-katex@git+https://github.com/iktakahiro/markdown-it-katex.git package.json package.json, pnpm-lock.yaml
Pull request alert summary
Issue Status
Install scripts ⚠️ 9 issues
Native code ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ⚠️ 1 issue
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

📊 Modified Dependency Overview:

➕ Added Package Capability Access +/- Transitive Count Publisher
[email protected] filesystem, shell, environment +22 akirami
[email protected] None +0 rundevelopment
⬆️ Updated Package Version Diff Capability Access +/- Transitive Count Publisher
[email protected] 12.8.1...12.9.0 eval, network, filesystem, shell, environment +8/-10 atofstryker
[email protected] 1.6.5...1.6.6 None +0/-0 simov
@fortawesome/[email protected] 6.2.1...6.4.0 None +1/-1 robmadole
@fortawesome/[email protected] 6.2.1...6.4.0 None +1/-1 robmadole
@fortawesome/[email protected] 6.2.1...6.4.0 None +1/-1 robmadole
@fortawesome/[email protected] 6.2.1...6.4.0 None +1/-1 robmadole

🚮 Removed packages: [email protected], [email protected], [email protected], [email protected], [email protected]

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

Doesn't Nuxt3 provide BuildIn Markdown render?

plugins/vueMarkdown.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Copy link
Member

@vikiival vikiival left a comment

Choose a reason for hiding this comment

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

Okish

I just have one problem with the markdown libratry that wasnt updated for 4+ years. Hope we will find workaround in next issue

@Jarsen136
Copy link
Contributor Author

Doesn't Nuxt3 provide BuildIn Markdown render?

Sorry, I do know this package before. I'm also not sure if it could support as much custom functionality as provided by vue-markdown like auto-link. Whatever, in this PR we unify all the markdown content display into a common component, so in the future, if we would like to replace it with another md library, that would be easier.

Copy link
Member

@preschian preschian left a comment

Choose a reason for hiding this comment

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

1 error on snyk, is it safe to ignore? I can't see the error message. don't have permission

security/snyk (kodadot) — 1 test has failed

@vikiival
Copy link
Member

Screenshot 2023-03-28 at 14 37 57

@vikiival
Copy link
Member

cc @yangwao can you please add us to snyk?

@vikiival
Copy link
Member

Looks like this was opened week ago.

cc @preschian @roiLeo do we proceed or closing this?

@yangwao
Copy link
Member

yangwao commented Mar 31, 2023

cc @yangwao can you please add us to snyk?

sent invite link, i was looking something different for snyk as their builds was not so sometimes ok and when they found some critical vuln they did not have it logs so itchy sketchy sometime

@yangwao
Copy link
Member

yangwao commented Mar 31, 2023

socket security > snyk actually (yet it's spamming bit to PRs :/)

@vikiival
Copy link
Member

Screenshot 2023-03-31 at 16 56 08

Screenshot 2023-03-31 at 16 56 05

@yangwao
Copy link
Member

yangwao commented Mar 31, 2023

this one is high severity and real deal

https://security.snyk.io/vuln/SNYK-JS-MARKDOWNITKATEX-597160

@yangwao
Copy link
Member

yangwao commented Mar 31, 2023

I'm ok here with it to take this one.
We can later re-evalute for new markdown libraries and make it proper implementation I guess, with Nuxt3 lot of new opportunities could be like with

- Also option is sanitze things, so XSS can't be executed

@Jarsen136
Copy link
Contributor Author

this one is high severity and real deal

https://security.snyk.io/vuln/SNYK-JS-MARKDOWNITKATEX-597160

Oh, I got it. Let me try to remove this package markdown-it-katex and find another better choice.

@Jarsen136 Jarsen136 marked this pull request as draft March 31, 2023 15:18
@yangwao
Copy link
Member

yangwao commented Mar 31, 2023

try to remove this package markdown-it-katex and find another better choice.

that's also way for it I guess! :)

@Jarsen136 Jarsen136 marked this pull request as ready for review March 31, 2023 16:52
@Jarsen136 Jarsen136 marked this pull request as draft March 31, 2023 16:52
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 5728 lines exceeds the maximum allowed for the inline comments feature.

@preschian
Copy link
Member

maybe try to transform markdown content with this package? https://mdxjs.com/

@Jarsen136
Copy link
Contributor Author

maybe try to transform markdown content with this package? https://mdxjs.com/

After some research, I found mdxjs is a good markdown content previewer to use on React, but not on Vue. It only supports mdx file render, but could not support converting md string content to vue element directly.
ref: https://mdx.nuxtjs.org/

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 5720 lines exceeds the maximum allowed for the inline comments feature.

@Jarsen136
Copy link
Contributor Author

Jarsen136 commented Apr 2, 2023

Eventually, it works as expected : )

This solution is here: miaolz123/vue-markdown#112 I use vue-markdown-v2 instead of vue-markdown to remove unsafe markdown-it-katex package.

For now, the snyk shows that there are three warning issue. I have overwritten these unsafe packages by the ones with safe version. So I think it's good to go now.

image

overwrite unsafe package:
image

@Jarsen136 Jarsen136 marked this pull request as ready for review April 2, 2023 12:31
@vikiival
Copy link
Member

vikiival commented Apr 2, 2023

Screenshot 2023-04-02 at 22 26 04

So why is snyk failing?

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 5720 lines exceeds the maximum allowed for the inline comments feature.

@vikiival
Copy link
Member

vikiival commented Apr 2, 2023

Also option is to fork the package and update it inside?

@codeclimate
Copy link

codeclimate bot commented Apr 2, 2023

Code Climate has analyzed commit 9b6dc63 and detected 0 issues on this pull request.

View more on Code Climate.

@Jarsen136
Copy link
Contributor Author

Screenshot 2023-04-02 at 22 26 04

So why is snyk failing?

I have mentioned the reason on the above comment. It's because snyk is not intelligent enough to know we have overwritten the package to the safe version. The snyk reports show that bothhighlight.js and markdown-it have unsafe versions. If you check the version at the package.json and pnpm.lock, you would find that we have overwritten the old version package.

Also option is to fork the package and update it inside?

I think yes. I would choose the forked package vue-markdown-v2 to solve it directly for now.

</template>

<script lang="ts" setup>
import VueMarkdown from 'vue-markdown-v2'
Copy link
Member

Choose a reason for hiding this comment

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

I think we can still use vue-markdown-render. that package support vue3 starting on v2 https://github.com/cloudacy/vue-markdown-render/releases/tag/v2.0.0. because vue-markdown-v2 was a bit outdated 4 years ago

I test it by changing it to this seems works:

<template>
  <vue-markdown :source="source" :options="options" />
</template>

<script lang="ts" setup>
import VueMarkdown from 'vue-markdown-render'
import hljs from 'highlight.js'

const props = defineProps<{
  source: string
  highlight?: boolean
}>()

const options = computed(() => {
  const defaultOptions = {
    html: true,
    linkify: true,
    typographer: true,
  }

  if (props.highlight) {
    return {
      ...defaultOptions,
      highlight: (code: string, lang: string) => {
        if (lang && hljs.getLanguage(lang)) {
          try {
            return hljs.highlight(lang, code).value
          } catch (error) {
            console.error(error)
          }
        }

        return hljs.highlightAuto(code).value
      },
    }
  }

  return defaultOptions
})
</script>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I will take a try later

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good 🤗

@vikiival
Copy link
Member

vikiival commented Apr 3, 2023

Hey,
since there is too much noise in this PR regarding many markdown packages.

I am considering to close this PR and make a new one.

WDYT @Jarsen136 @preschian ?

@Jarsen136
Copy link
Contributor Author

Hey,
since there is too much noise in this PR regarding many markdown packages.

I am considering to close this PR and make a new one.

WDYT @Jarsen136 @preschian ?

OK, Let's close it. And I will raise a new PR soon

@vikiival vikiival closed this Apr 3, 2023
@floyd-li floyd-li mentioned this pull request Apr 3, 2023
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autolink in item description
6 participants