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 marked instead of roaster + update marked #1161

Merged
merged 9 commits into from Sep 30, 2020
Merged

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Sep 23, 2020

Description of the Change

This:

  • Uses marked instead of roaster which was deprecated years ago.
  • Updates marked to the latest version

Benefits

  • The old roaster was deprecated years ago and can bring many bugs and security issues.
  • The new marked has many improvements to its engine. Follows the latest GitHub behavior and has fixed a huge number of bugs, security issues, etc.

Possible Drawbacks

N/A

Applicable Issues

Closes #1148
Fixes #500

@aminya aminya marked this pull request as draft Sep 23, 2020
If true, add <br> on a single line break (copies GitHub behavior on 
comments, but not on rendered markdown files). Requires gfm be true.
@aminya
Copy link
Contributor Author

aminya commented Sep 23, 2020

@UziTech You are one of the main "marked" developers. It would be nice if you review this!

GitHub does not break line in a single line anymore. Should we update the failing test?
This is based on: markedjs/marked#1620.

@UziTech
Copy link
Contributor

UziTech commented Sep 23, 2020

Github does support single line breaks on comments but not on readmes. I would do the same thing here. Enable line breaks for descriptions but not on the readme content.

I noticed that marked is being used in an asynchronous way, but since marked doesn't need to connect to the file system or database it still just runs on the main thread, which can be bad for performance and lead to ReDos attacks. We should run marked in a worker or Task wherever we want it to be off the main thread.

To follow GitHub's behavior
@aminya
Copy link
Contributor Author

aminya commented Sep 23, 2020

Github does support single line breaks on comments but not on readmes. I would do the same thing here. Enable line breaks for descriptions but not on the readme content.

Does this mean what you are suggesting?

I noticed that marked is being used in an asynchronous way, but since marked doesn't need to connect to the file system or database it still just runs on the main thread, which can be bad for performance and lead to ReDos attacks. We should run marked in a worker or Task wherever we want it to be off the main thread.

We need to merge atom/atom#21139 to be able to use WebWorkers. I think Task would be expensive here as it would start a new fork for every rendering.

After we merge atom/atom#21139, we can definitely use WebWorkers for this.

@UziTech
Copy link
Contributor

UziTech commented Sep 23, 2020

Does this mean what you are suggesting?

yes

I think Task would be expensive here

I agree we should wait for web workers

@UziTech
Copy link
Contributor

UziTech commented Sep 23, 2020

It looks like the tests fail because the renderer is removing html but the test expects <br/> to be allowed. marked fixed inline html not going through the html renderer which is why it is breaking when updating marked.

The solution would be to not allow all html or update the renderer to allow <br/>.

@aminya
Copy link
Contributor Author

aminya commented Sep 24, 2020

I added br method. If it does not work, I think we need to change html method to something like this:

renderer.html = (src) => {
	const match = src.match(/<br\/>/);
	if (match) {
		return `<br/>`
	}
	return ''
}

@aminya aminya marked this pull request as ready for review Sep 24, 2020
@aminya
Copy link
Contributor Author

aminya commented Sep 24, 2020

@sadick254 @darangi This is ready to go! 😃 All the tests pass.

lib/rich-description.js Outdated Show resolved Hide resolved
Co-authored-by: Tony Brix <tony@brix.ninja>
@sadick254 sadick254 merged commit 2a29077 into atom:master Sep 30, 2020
2 checks passed
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.

Checkboxes in package Readmes are rendered as Settings View checkboxes
3 participants