Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Ideas to support SVG primarily #29

Closed
wants to merge 16 commits into from

Conversation

andrewgryan
Copy link

@andrewgryan andrewgryan commented May 28, 2023

Ready for review

An idea to extend the current usage of VanJS with respect to van.tags to allow for SVG element custom document.createElementNS API.

// Original behaviour preserved
let { div } = van.tags

// Extended behaviour
let { svg, circle } = van.svgs

And the custom NS is available too by supplying a URI to the tagsNS function.

let { foo } = van.tagsNS("uri")

This PR may also change to reflect the proceedings of the discussion board if desired by the maintainers of the project.

Update: Further simplifcation of design to improve developer experience.

@iongion
Copy link

iongion commented May 28, 2023

This is cool, looking for something like this to exist already ❤️

@andrewgryan
Copy link
Author

I'm pretty happy with the design, just wondering how to test it and update the types/docs. Although that might be a chore for the maintainers, I'm happy to do it but can't figure out a good way to run the tests

@Tao-VanJS
Copy link
Member

The draft development and testing process was described here: #2 (comment). I plan to add the process to README.md after verified by some contributor.

Regarding this specific PR, I'm still leaning towards to the design of:

const {div, p} = van.tags
const {circle, svg} = van.tagsNS("http://www.w3.org/2000/svg")

The pros are:

  • Non-breaking
  • Close to current VanJS API
  • tags and tagsNS aligns with the style of native DOM (createElement and createElementNS)

@andrewgryan
Copy link
Author

I agree tagsNS is a tempting solution, and alignment with the DOM spec is a noble cause but I feel there's a better way. I've implemented tagsNS in the latest commit so that we can take a look at the problem from a user's point of view.

Consider for a moment the API I expected to use that created the problem, I typed it without thinking and it is the path of "least surprise", sadly it's hard to implement while maintaining VanJS's design principles.

// Best possible API, but unfortunately not trivial to implement flawlessly/without bloat
const { div, svg, ellipse } = van.tags

My guess is the above API would ruin the "smallest" lib unique selling point. The solution in my mind is to detect each known svg tag and call createElementNS instead of createElement, which would require keeping a list of all supported svg tag names in memory. This might also be difficult to future proof, say a new SVG tag is supported by browser vendors, we would then mispredict and call createElement erroneously.

In JSX frameworks, the <svg /> tag works out of the box, the namespace string is an arcane piece of knowledge that very few web developers know about or indeed "need" to know about. For a library that wants to be "the scripting language of UI" I feel forcing people to look up long forgotten corners of the document object model is a poor design choice considering other web frameworks don't use it.

// SolidJS tutorial docs (trimmed to fit here)
<svg height="300" width="400">
  <ellipse cx="125" cy="150" rx="100" ry="60"  />
</svg>

The thing that drew me to VanJS in the first place was it's design philosophy and excellent developer experience. It was really easy to pick up and build with and felt cool that it was only a few bytes.

The two API choices on the table (both implemented) can be compared below. One was easy to type the other requires knowledge of a seasoned web developer (actually I spent hours flailing before figuring it out).

const { svg, ellipse } = van.elements.svg
const { svg, ellipse } = van.tagsNS("http://www.w3.org/2000/svg")  // I couldn't type this, I copied from above

As any project, the maintainer has the final say on what goes in but I think there's an opportunity here to not repeat the API mistakes of the past (looking at you DOM spec) and focus on the design principles that make VanJS an awesome project to work with.

Also, thanks for pointing me to the testing instructions, I'll get to them as soon as I can, my in-laws are visiting :-)

@andrewgryan
Copy link
Author

I've removed the .elements object that I was using to group element factory usages in favour of an even simpler API.

const { svg, circle, ellipse } = van.svgs;

This feels about the same as the current API for HTML tags and avoids potentially 132kb of unzipped memory to support seemless HTML/SVG usage. For a tiny bit of extra typing we get all of the SVG elements behind a Proxy to keep the ~1kb compressed selling point

@andrewgryan
Copy link
Author

Completed the test/build instructions with minor modifications to publish.sh, probably not appropriate for this PR. I'll move on to adding coverage for the new functionality soon.

@andrewgryan andrewgryan changed the title WIP Ideas to support SVG primarily Ideas to support SVG primarily May 30, 2023
@andrewgryan
Copy link
Author

@Tao-VanJS I've reverted the changes related to files affected by publish.sh and the package.json version bump. I've cherry-picked the bug-fix so that the test suite works. I'm happy for you to review this, while I'm waiting (timezone difference) I'll see if I can easily add tests to check the change. I can also submit tests in a follow-up PR if you're keen to get SVG support into the library.

@andrewgryan
Copy link
Author

@Tao-VanJS should van.d.ts also be updated with the "known" SVG tags to help people's intellisense in their editors?

@andrewgryan
Copy link
Author

Actually, adding types feels like a separate pull request. There are quite a few.

https://developer.mozilla.org/en-US/docs/Web/API/Document_Object_Model#svg_interfaces

@Tao-VanJS
Copy link
Member

This PR was manually merged as part of the 0.12.0 release. I tried to add @andrewgryan as the co-author of the commit but somehow @andrewgryan isn't shown as the contributor of the repo. Nonetheless, I added @andrewgryan manually in the contributor list of the README.md file.

@Tao-VanJS Tao-VanJS closed this Jun 8, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants