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

Enable JSX support in frontend #15293

Closed
wants to merge 1 commit into from
Closed

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Apr 5, 2021

This enables frontend to use JSX to generate DOM nodes using the jsx-dom module. JSX is inherently safer than concatenating HTML strings because it defaults to HTML escaping. Also it is easier to work with and more powerful than template strings.

To demonstrate JSX usage, I've rewritten the context popup feature to use it, looks pretty much exactly as before:

Screen Shot 2021-04-05 at 22 25 07

Generally the aim of this feature is to provide a safe and convenient alternative to string concatenation to generate DOM elements. I plan to migrate more string usage to this method after this is merged.

@silverwind silverwind added topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile topic/ui Change the appearance of the Gitea UI skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Apr 5, 2021
@silverwind silverwind added this to the 1.15.0 milestone Apr 5, 2021
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Apr 9, 2021
@silverwind
Copy link
Member Author

silverwind commented Apr 10, 2021

Switched to vhtml which is a more lightweight JSX rendering alternative that outputs strings instead of DOM elements which I think is more suitable to our general use cases. Escaping is still taken care automatically and we save around 8kB unminified bundle size with it compared to jsx-dom.

@silverwind
Copy link
Member Author

Some more refactors done. Moved the JSX components to its own file and added a informational comment about them.

I've been attempting to test the components but could not get the JSX tranformation to work with the unstable version of Jest, so I guess we'll have to wait a bit for jest to stabilize their ESM features.

@silverwind
Copy link
Member Author

I guess I may actually switch it back to jsx-dom because one can not bind event handlers on strings which would prove inflexible later on. Putting on WIP until then.

@silverwind silverwind added the pr/wip This PR is not ready for review label Apr 10, 2021
@lunny
Copy link
Member

lunny commented Apr 11, 2021

Why not just continue with vue but switch to JSX ?

@silverwind
Copy link
Member Author

silverwind commented Apr 12, 2021

Vue/React are for SPA-style rendering (e.g. build DOM elements and render/update). Only few isolated components in the UI use this type of renderning.

This JSX usage is for cases where we only construct DOM elements/HTML and insert them manually (.innerHTML or jquery .html()). There are many such cases in the codebase and they are inherently unsafe with user-generated content unless properly escaped. Using JSX makes this type of rendering both easier and safe from escaping-related errors.

@silverwind
Copy link
Member Author

silverwind commented Apr 12, 2021

Using Vue JSX later on should be possible, we just need to define multiple jsxFactory functions and do filename-based matching to route different files to different JSX transformers. Thought it seems Vue depends on Babel for their JSX transform which I'd rather not like to re-introduce, so we probably can't have that.

@silverwind silverwind removed the pr/wip This PR is not ready for review label May 17, 2021
@silverwind
Copy link
Member Author

Switched back to jsx-dom and cleaned up a few more things to keep the diff minimal. Ready again.

@lunny
Copy link
Member

lunny commented May 20, 2021

CI still failed.

This enables frontend to use JSX to generate DOM nodes using the
`jsx-dom` module [1]. JSX is inherently safer than concatenating HTML
strings because it defaults to HTML escaping. Also it is easier to work
with and more powerful than template strings.

To demonstrate JSX usage, I've rewritten the context popup feature to
use it, no functional changes there.

[1]: https://github.com/proteriax/jsx-dom
@silverwind
Copy link
Member Author

Can't be related, rebased.

@lafriks
Copy link
Member

lafriks commented May 22, 2021

I would also prefer that these components be converted to Vue instead of jsx, we already have way too big frankenstein in fronted to introduce yet another concept

@silverwind
Copy link
Member Author

silverwind commented May 22, 2021

I'm not going to do Vue, sorry. I could easily convert all current string-built HTML to JSX but to me Vue is a framework I deeply disagree with and I'd rather like to get rid of it in favor of React.

The JSX currently returns plain DOM elements but could easily be converted to React later. Maybe also migrate the existing Vue components to React.

I think JSX is by far the best way of representing HTML in JS, there are no strange .vue files that are some odd mix of HTML/CSS/JS but proper JS via the help of a transformation.

@lafriks
Copy link
Member

lafriks commented May 22, 2021

I will agree to disagree 😸 I'm on the other hand don't like react very much but everyone has their own favorites and I respect that 😉
I just don't like when JS code get mixed with html so much that it reminds me old PHP code 😅

@silverwind
Copy link
Member Author

silverwind commented May 22, 2021

Well if you really want to keep it to Vue, we could use the Vue JSX factory function, e.g. https://github.com/vuejs/jsx-next#content. It looks exactly like a React functional component in the simplest variant and close to it in the other variants, they Vue has apparently been copying hard from React on that part.

I think using that JSX means current components need to migrate to Vue 3 because Vue 2 does not export the necessary factory functions. It'd be a mess which would probably have to load Vue twice.

I just don't like when JS code get mixed with html so much that it reminds me old PHP code 😅

I'd count that as a benefit of PHP. I enjoy being able to use JS functional methods like .map to compose HTML and this JSX transformer allows just that, nothing more. It should be easy to switch from this to either Vue or React JSX later.

@lafriks
Copy link
Member

lafriks commented May 22, 2021

That's to be honest is exactly what I dislike about react most is mixing js with html 😕

@lunny
Copy link
Member

lunny commented May 23, 2021

The pure VUE is a framework like jquery, it's very small. I think the .vue file is better than js or JSX syntax because it's more clear to developers, it will not mix different type codes.

I agree we should switch to vue3 but it's for better performance and smaller size.
And also maybe vite which also based on esbuild and have the most fastest development speed.

@silverwind
Copy link
Member Author

I still want to replace all the current places that construct HTML from strings with JSX, I don't really care whether it's Vue or something else that is processing the JSX as long as the output format of DOM nodes or string is possible. Vue 3 (finally) embraces JSX and I think it's the future, so I prefer if we start converting the existing SFCs to Vue 3 JSX.

@silverwind
Copy link
Member Author

silverwind commented May 24, 2021

maybe vite

Not sure whether that supports all our current webpack use cases. Stuff like license file generation may not be possible. Webpack is certainly much more flexible in terms of extensibility.

@silverwind
Copy link
Member Author

That's to be honest is exactly what I dislike about react most is mixing js with html 😕

Do you dislike React or dislike JSX? I think it's a weak argument. Composing HTML parts from JS just feels natural to me and it's already practiced in many places in the current code base because it is essentially the only possible way.

@silverwind
Copy link
Member Author

silverwind commented May 24, 2021

The pure VUE is a framework like jquery, it's very small

The same is true for react-dom, it's size, while slightly bigger than Vue is not much of a concern:

https://bundlephobia.com/result?p=react@17.0.2
https://bundlephobia.com/result?p=react-dom@17.0.2
https://bundlephobia.com/result?p=vue@3.0.11

@lafriks
Copy link
Member

lafriks commented Jun 1, 2021

That's to be honest is exactly what I dislike about react most is mixing js with html confused

Do you dislike React or dislike JSX? I think it's a weak argument. Composing HTML parts from JS just feels natural to me and it's already practiced in many places in the current code base because it is essentially the only possible way.

Is both an answer? :) But yeah I dislike JSX mostly because of mixed JS and HTML that makes it unbearably unreadable. And react for it's bulkiness and some of it's design decisions.

I like vue that's just like go is plain and simple, maybe I'm too old but I'm too tired of complex languages that makes it hard to maintain in the end, I have seen too many such systems in last 20 years 😆

@techknowlogick techknowlogick modified the milestones: 1.15.0, 1.16.0 Jun 23, 2021
@lafriks
Copy link
Member

lafriks commented Jul 14, 2021

Only change here still needed is moving contrast color function to utils.js. @silverwind can you move it to other PR?

@lafriks lafriks added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Jul 14, 2021
@silverwind
Copy link
Member Author

Yes I can extract that later.

@wxiaoguang
Copy link
Contributor

I vote for Vue3 (Vue is already used in this project) + JSX (nice to have)

@wxiaoguang wxiaoguang mentioned this pull request Sep 25, 2021
@lunny lunny modified the milestones: 1.16.0, 1.17.0 Nov 15, 2021
@silverwind
Copy link
Member Author

Closing as this won't happen.

@silverwind silverwind closed this Nov 23, 2021
@lunny lunny removed this from the 1.17.0 milestone Nov 24, 2021
@wxiaoguang
Copy link
Contributor

I think it is good, if more people accept it, maybe we can welcome it back in future.

@silverwind
Copy link
Member Author

silverwind commented Nov 24, 2021

Yeah, I'm still in favor of JSX for inline HTML snippets. We should at least have something that auto-escapes inserted values (but also provides a way to intentionally have unescaped content like SVGs).

GitHub apparently uses https://github.com/lit/lit/tree/main/packages/lit-html for this, but I'm not sure it's suitable because I see no way to introduce unescaped content. Also, it seems to bloated in case one only uses the template function, we effectively get the same by using htmlEscape as tagged template function.

@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants