Skip to content
This repository has been archived by the owner on Sep 19, 2024. It is now read-only.

feat: change to markdown and map html to markdown #688

Draft
wants to merge 7 commits into
base: development
Choose a base branch
from

Conversation

whilefoo
Copy link
Collaborator

@whilefoo whilefoo commented Aug 29, 2023

Resolves #673

QA: ubiquity-whilefoo#56

@netlify
Copy link

netlify bot commented Aug 29, 2023

Deploy Preview for ubiquibot-staging ready!

Name Link
🔨 Latest commit 8e6ddaa
🔍 Latest deploy log https://app.netlify.com/sites/ubiquibot-staging/deploys/651db82cce1e250008a3ca1b
😎 Deploy Preview https://deploy-preview-688--ubiquibot-staging.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.

// dynamic import of mdast
const { fromMarkdown } = await import("mdast-util-from-markdown");
const { gfmFromMarkdown } = await import("mdast-util-gfm");
const { gfm } = await import("micromark-extension-gfm");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried importing normally at the top but I got the error:

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/whilefoo/ubiquity/ubiquibot/node_modules/mdast-util-from-markdown/index.js from /Users/whilefoo/ubiquity/ubiquibot/lib/src/helpers/comment.js not supported.
    Instead change the require of index.js in /Users/whilefoo/ubiquity/ubiquibot/lib/src/helpers/comment.js to a dynamic import() which is available in all CommonJS modules.

After that I changed to dynamic import in the function but the error stays the same. I checked the compiled javascript and it's still require() - it seems every import is compiled to require.

@rndquu any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

I guess this is a related issue #378

And this is a related PR #377 (which had been reverted because we got rid of mdast packages)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it seems #377 was supposed to fix the issue but it doesn't - instead of throwing the error at startup, it throws the error when the function runs.

Copy link
Member

Choose a reason for hiding this comment

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

So it throws the same error both when modules are imported at the top of the comment.ts file and in the parseComments() function, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So it throws the same error both when modules are imported at the top of the comment.ts file and in the parseComments() function, right?

yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried changing Typescript settings to transpile to ESM and added "type": "module" to package.json, but I get a different error because Probot is using require() internally

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/whilefoo/ubiquity/ubiquibot/lib/src/index.js from /Users/whilefoo/ubiquity/ubiquibot/node_modules/probot/lib/helpers/resolve-app-function.js not supported.
Instead change the require of index.js in /Users/whilefoo/ubiquity/ubiquibot/node_modules/probot/lib/helpers/resolve-app-function.js to a dynamic import() which is available in all CommonJS modules.

node_modules/probot/lib/helpers/resolve-app-function.js is using require()

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we could downgrade all 3 packages (mdast-util-from-markdown, mdast-util-gfm, micromark-extension-gfm) to their "pre ESM" versions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried downgrading and I have an issue with mdast-util-gfm because that version didn't have any types so typescript is complaining. I will see if there's anything I can do

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/ubiquity/ubiquibot/blob/incentive-based-on-comment/src/helpers/comment.ts#L23

Please take a look at the incentive-based-on-comment branch @whilefoo. It has been done with mdast library.
The blocker on that branch was the function timeout: 10s as of now.

I hope you can get reusable/helpful methods from that branch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please take a look at the incentive-based-on-comment branch @whilefoo.

That is exactly how I've done it but it's not working, you can check the whole conversation for context

[MarkdownItem.LinkReference]: HTMLItem.A,
[MarkdownItem.ImageReference]: HTMLItem.IMG,
[MarkdownItem.FootnoteReference]: HTMLItem.SUP,
[MarkdownItem.FootnoteDefinition]: HTMLItem.P,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure how can we represent footnote reference and definition by HTML? @pavlovcik

Example of footnote1

Footnotes

  1. Footnote 1

Copy link
Member

Choose a reason for hiding this comment

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

Very cool. I didn't know about this but I guess we can just file an issue and kick the can down the road?

[MarkdownItem.Link]: HTMLItem.A,
[MarkdownItem.Image]: HTMLItem.IMG,
[MarkdownItem.BlockQuote]: HTMLItem.BLOCKQUOTE,
[MarkdownItem.Code]: HTMLItem.PRE,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if it's gonna be clear to partners that <pre> refers to

code

and <code> refers to inline code

@whilefoo
Copy link
Collaborator Author

whilefoo commented Sep 3, 2023

@pavlovcik can you clarify if task creator and task assignee are eligible for task conversation rewards? I'd guess not because task creator will make comments to clarify his task and task assignee will comment to explain his solution or ask questions to the task creator

@rndquu
Copy link
Member

rndquu commented Sep 4, 2023

@pavlovcik can you clarify if task creator and task assignee are eligible for task conversation rewards? I'd guess not because task creator will make comments to clarify his task and task assignee will comment to explain his solution or ask questions to the task creator

Oh, I actually thought that task creator is also eligible for task conversation rewards. Ok then, I will update the description for #704

@0x4007
Copy link
Member

0x4007 commented Sep 4, 2023

@pavlovcik can you clarify if task creator and task assignee are eligible for task conversation rewards? I'd guess not because task creator will make comments to clarify his task and task assignee will comment to explain his solution or ask questions to the task creator

That's a good observation. Here are all the roles that I see and if they should be eligible for comment incentives. What if we make them configurable?

Something like:

### on issue view

- assignee: false
- issuer: false
// - reviewer: n/a
- default: true

### on pull request view 

- assignee: false
- issuer: true
- reviewer: true
- default: true

@0x4007
Copy link
Member

0x4007 commented Sep 26, 2023

Updated roles:

Issue

  • assignee: false
  • issuer: false
    // - collaborator: n/a
  • contributor: true

Review

  • assignee: false
  • issuer: true
  • collaborator: true
  • contributor: true

Debating on if we should make a "reviewer" role which kicks in if you helped with the review. These are who need to be highly incentivized because review seems to be very burdensome.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conversation Rewards Hot Fixes
4 participants