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

Change internal VNode data structure. #578

Closed
jorgebucaran opened this issue Jan 29, 2018 · 11 comments
Closed

Change internal VNode data structure. #578

jorgebucaran opened this issue Jan 29, 2018 · 11 comments

Comments

@jorgebucaran
Copy link
Owner

jorgebucaran commented Jan 29, 2018

I'd like to discuss the possibility to change our internal VNode data structure name and props fields to nodeName and attributes respectively.

This is an internal breaking change, so I don't believe it warrants a major bump. If you know semver better than I do, can you please help me confirm this?

Before

{
  name: "h1",
  props: { id: "title" },
  children: "Hello"
}

After

{
  nodeName: "h1",
  attributes: { id: "title" },
  children: "Hello"
}

Why?

It makes Hyperapp VNode object compatible with Preact and possibly other libraries that use the same schema. I think React uses elementName instead, which is a shame.

This is useful if you are authoring a tool that outputs virtual nodes (e.g., @lukejacksonn's ijk or a JSX plugin that transforms JSX to VNodes skipping hyperscript syntax).

Maybe you want the library to be compatible with Hyperapp out of the box without having to distribute a Hyperapp-specific build or use a constructor function to register the VNode schema, but you also want to support at least another framework such as Preact.

Related

@jorgebucaran jorgebucaran added discussion API breaking This will break things labels Jan 29, 2018
@SkaterDad
Copy link
Contributor

No objections here. As you say, this is an internal change, so it doesn't really affect users unless they were creating their own raw VNodes.

I'm not seeing how you could intermingle hyperapp and preact vnodes in a useful way, though? Any examples of how & why that would be done?

Or is the idea that stateless functional components from preact could be used directly in a hyperapp view? Such as a purely stylistic UI library.

@lukejacksonn
Copy link
Contributor

@SkaterDad your final statement..

that stateless functional components from preact could be used directly in a hyperapp view

and vice versa.. It would be nice if the views (stateless at least) were not tied to a specific vdom implementation.

Further food for thought is trying to rally together all vnode implementations and establishing an industry/community standard. Not sure exactly how sensible/realistic that is but it seems logical!

@rbiggs
Copy link
Contributor

rbiggs commented Jan 30, 2018

Not sure if you need to make this kind of a change for stateless functional components. They are already mostly transparent across React, Inferno, Preact and even Picodom because its the h function that does the transformation for the particular library. I have moved stateless functional components between all four and more without a problem, so changing an internal name used by Hyperapp's h function won't affect this type of reuse of functional components. Nobody creates a VNode from scratch, they use a hyperscript-compatible function to do that.

The only gotcha in reusing functional components is the React-specific attribute convention: className for class, onClick for onclick. Preact checks the props and converts React props into Preact equivalents. I do the same with a render function for Picodom. So no sweat.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Jan 30, 2018

@SkaterDad Take @hyperapp/html for example. It uses a peer dependency to Hyperapp for hyperapp.h to produce VNodes. If we removed that peer dependency and spit raw nodes, and also the schema of those node was the same as Preact, then we could use @hyperapp/html with Preact without having to deploy two different builds: one with hyperapp as a peer dep and another with preact.

@brodycj
Copy link

brodycj commented Jan 31, 2018

Why not target Inferno VNodes, considering that Inferno is one of the most performant frameworks according to https://github.com/krausest/js-framework-benchmark?

@rbiggs
Copy link
Contributor

rbiggs commented Jan 31, 2018

Inferno, like React, uses the term type for the node type in contrast to nodeName in Preact and name in Hyperapp. Jorge, why not just check for the name time so that it works for name, nodeName and type? Then it would work with all of them.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Jan 31, 2018

@brodybits Performance is not the most important metric when choosing a framework. It's not even close to the top in my book. But if Inferno and React use the same VNode, then Inferno+React > Preact IMO.

@jorgebucaran
Copy link
Owner Author

@rbiggs You mean a runtime check no? If so, no, that would kind of defeat this initiative.

@sergey-shpak
Copy link
Contributor

sergey-shpak commented Feb 2, 2018

In case anyone needs backward thing (to generate compatible vNode structure with hyperscript)

import h from the following module

import { h as _h } from 'hyperapp';

export const h = (...args) => {
  const { name, props, children } = _h(...args);
  return { name, props, children, nodeName: name, attributes: props }
}

@rbiggs
Copy link
Contributor

rbiggs commented Feb 2, 2018

Yeah, I looked at the source code. Not gonna work. One thing, Hyperx just takes the library's h function to return a factory. But I don't see any easy way to do that for all of the tags.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Feb 7, 2018

Removing breaking, because this is not technically a breaking change if we consider breaking changes only changes that break the external / public API, which this is not.

@jorgebucaran jorgebucaran removed the breaking This will break things label Feb 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants