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

Switch basichtml with linkedom #27

Merged
merged 3 commits into from
Jul 4, 2021
Merged

Switch basichtml with linkedom #27

merged 3 commits into from
Jul 4, 2021

Conversation

Kal-Aster
Copy link
Contributor

basicHTML became deprecated, recommending linkedom as alternative.
This PR does this switch.

@GianlucaGuarini GianlucaGuarini self-assigned this Jul 1, 2021
@GianlucaGuarini
Copy link
Member

GianlucaGuarini commented Jul 1, 2021

That's a good patch, it needs some work though. I would prefer cleaning up the globals once the [dispose]
(https://github.com/riot/ssr/blob/main/src/index.js#L50-L55) function gets called. I think we can create and destroy them in the render function avoiding polluting the global scope.

@Kal-Aster
Copy link
Contributor Author

I can do this

@Kal-Aster
Copy link
Contributor Author

BTW in my project I found myself needing the createRenderer function for a custom render flow and I found that it is not exported.

Do you think that it's a good idea exporting it, for the sake of extendability?
Can I do it in this PR or should I open a new one?

@GianlucaGuarini
Copy link
Member

That's a good Idea but I would rather create a separate PR for it

@GianlucaGuarini GianlucaGuarini merged commit 0115883 into riot:main Jul 4, 2021
@GianlucaGuarini
Copy link
Member

thank you I will release the new major release together with Riot.js 6

@Kal-Aster
Copy link
Contributor Author

Wow, Riot.js 6? I haven't seen any news about it, is there any expected release date?

However thank you, I'll PR for exporting createRenderer.

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.

2 participants