atom / settings-view

🔧 Edit Atom settings
MIT License
273 stars 275 forks source link

Use marked instead of roaster + update marked #1161

Closed aminya closed 3 years ago

aminya commented 3 years ago

Description of the Change

This:

Benefits

Possible Drawbacks

N/A

Applicable Issues

Closes #1148 Fixes #500

aminya commented 3 years ago

@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: https://github.com/markedjs/marked/pull/1620.

UziTech commented 3 years ago

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.

aminya commented 3 years ago

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 https://github.com/atom/atom/pull/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 https://github.com/atom/atom/pull/21139, we can definitely use WebWorkers for this.

UziTech commented 3 years ago

Does this mean what you are suggesting?

yes

I think Task would be expensive here

I agree we should wait for web workers

UziTech commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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