TypeScript Conversion

[pspeter3]

This looks really great. How would you feel about converting to TypeScript? (happy to help)

[jorgebucaran]

@pspeter3 Let's do it!

[jorgebucaran]

Ping @ngryman! 💪

[qheolet]

I will like to help

[sumeetsarkar]

Sure! I will like to help too

[pspeter3]

Alright, I think that in order to convert to TypeScript, there are a few things that we should do:

• [ ] Update to NPM5 and update dependencies The logic here is that we should have the code working with the newest versions of things. NPM5 lock files are nice and prettier@1.4.0 supports TypeScript. I have a working example of that.
• [ ] Convert core code to TypeScript I can work on this. I think the largest difficulty is just getting the build and publish chain correct.
• [ ] Add a JSX namespace In order for TSX to work, a JSX namespace must exist with precise types for all of the HTML attributes. I actually think this is the most important work and the one that needs the most help. The elements could be split amongst people.
• [ ] Add tests and coverage At this point, tests and coverage which would be awesome

Thoughts?

[jorgebucaran]

@pspeter3 Looking good, but can we also elaborate on the advantages and disadvantages of rewriting the project in TypeScript?

Also, picodom is advertised as a 1 KB library and that is one of the project's main appeals. Do you know if rewriting everything in TypeScript could yield a bundle size >1 KB? 🤔

[jorgebucaran]

With regards to tests: See #3.

As for the implementation, we can learn from hyperapp here:

• https://github.com/hyperapp/hyperapp/tree/master/test

[pspeter3]

@jbucaran Great questions! My belief is that compiling to ES6 as a target would maintain the same code size while compiling to lower targets such as ES3 would increase code size but allow backward compatibility.

Pros

• Type safe code which should remove common bugs, especially around null checking
• First class support for other TypeScript projects. Most projects rely on hand written TypeScript definitions as opposed to the definitions generated by other compilers
• Backwards syntax compatibility for ES3 and ES5 if necessary
• Self documenting API

Cons

• Slightly more knowledge required to contribute
• Additional build step

[jorgebucaran]

Thanks @pspeter3. Another con is:

• Less visibility (JavaScript projects tend to trend and gain more visibility on GitHub).

However, I'd still like to move forward with this, and see where it leads us.

[pspeter3]

Sounds good, I'll work on converting core and then we can add the jsx name space

[jorgebucaran]

@pspeter3 BTW, before a full convert, we could also try introducing flow or add .ts typings. Just throwing ideas.

[jorgebucaran]

Given this library, https://github.com/TylorS167/mostly-dom, is converting worth it?

I had a look. Don't see how it affects/relates to picodom. 🤔

[pspeter3]

I agree, that's why I deleted the comment. It just seemed like another TypeScript VDOM technology

[ngryman]

I will probably sound like an old grumpy granpa but honestly I don't see any point in using Typescript for such a project, and generally I don't understand why all the fuzz around it.

I won't start a whole debate here, but JavaScript is what it is: a multi-paradigm language (functional / imperative) leveraging OLOO. It's not an OO language and even if ES6 sadly brings class, it will never be. It has its advantages and disadvantages, but once you've embraced / mastered it, it's extremely powerful and any attempt to twist it so it looks like Java/C# is just a way to ease those developers transition to the Web platform IMO. But it also prevents them to really learn JavaScript and its strengths. Plus, the nerd in me would say it's not an open standard so it's not made to last. ESNext will.

The only advantage I can see in using Typescript would be static type checking (which can be achieved by other means). For a 10k+ LOC project with dozen of developers collaborating it would make sense, as humans make errors and type checking avoids a bunch of them. But for such a small, focused, performance-driven project, I don't see any advantage, except verbosity.

I don't mean to offend anyone here, those are my opinions and I hope you don't take anything personally @pspeter3.

If you need type definitions, let's just add them aside no?

[jorgebucaran]

I agree with @ngryman. So, to make the cake we will need the following ingredients:

• Static type checking (Flow?)
• Type definitions (typings.json, *.d.ts)

@pspeter3 Would you still be interested in collaborating on picodom if we call off the Typescript rewrite?

[pspeter3]

While I disagree with your reasoning about the costs and benefits, I respect your decision and appreciate your honest and thoughtful viewpoint. I personally am not very motivated to work on the type definitions. My experience from maintaining the React type definitions and working with other virtual DOM implementations is that the type definitions easily get out of sync with the codebase. The largest benefit I was hoping for was to have that not be possible.

Thank you once again!

[jorgebucaran]

@pspeter3 I agree with you on the type definitions, and I would wait until things are more stable (stable atm if you ask me though). What about flow?

While I disagree with your reasoning about the costs and benefits...

I'm not an expert, so I'd love to hear your reply to @ngryman's comments. We're all learning here. I'm learning.

[ngryman]

I'm not an expert, so I'd love to hear your reply to @ngryman's comments. We're all learning here. I'm learning.

True, and I'd like to hear if I've missed something.

Again I don't really want to debate if X is better than Y, that's not the point. But if you have a strong experience with Typescript (which I don't) and you see real advantages that I failed to see, I'm totally open and curious to get that knowledge.

[pspeter3]

I think the biggest point of contention is that TypeScript encourages you to write code in a certain way. TypeScript is a super set of JavaScript so the code can be nearly identical, see the examples below. While I agree with TypeScript not being an open standard, there is talk of adding type annotations to ES7, and it is always possible to compile the TypeScript code to JavaScript and delete the original TypeScript.

With regards to benefits, the benefits are the easy type definitions and the ability to catch bugs. For example, the compiler points out that we have not handled the case of recursive components in the h function. Note: I may have just misunderstood the h function so it is possible it was handled correctly elsewhere

h.ts

export type Primitive = string | number | boolean;

export type Child = VElement<string, {}> | Primitive | null;

export type Children = Child[];

export interface VElement<T extends string, D> {
    tag: T;
    data: D;
    children: Children;
}

export type Fragment = Child | Child[];

export type VNode<T extends string, D> = VElement<T, D> | Component<D>;

export interface Component<D> {
    (data: D, children: Children): VNode<string, {}>;
}

export function h<T extends string, D>(tag: T | Component<D>, data: D, ...stack: Fragment[]): VElement<string, {}> {
    let node: Fragment;
    const children: Children = [];
    while (stack.length) {
        node = stack.pop();
        if (Array.isArray(node)) {
            for (let i = node.length; i--;) {
                stack[stack.length] = node[i];
            }
        } else if (node !== null && node !== true && node !== false) {
            if (typeof node === "number") {
                node = node + "";
            }
            children[children.length] = node
        }
    }

    return typeof tag === "string"
        ? { tag, data, children }
        : tag(data, children);
}

Generated h.js

export function h(tag, data) {
    var stack = [];
    for (var _i = 2; _i < arguments.length; _i++) {
        stack[_i - 2] = arguments[_i];
    }
    var node;
    var children = [];
    while (stack.length) {
        node = stack.pop();
        if (Array.isArray(node)) {
            for (var i = node.length; i--;) {
                stack[stack.length] = node[i];
            }
        }
        else if (node !== null && node !== true && node !== false) {
            if (typeof node === "number") {
                node = node + "";
            }
            children[children.length] = node;
        }
    }
    return (typeof tag === "string"
        ? { tag: tag, data: data, children: children }
        : tag(data, children));
}

Original h.js

export function h(tag, data) {
  var node
  var stack = []
  var children = []

  for (var i = arguments.length; i-- > 2; ) {
    stack[stack.length] = arguments[i]
  }

  while (stack.length) {
    if (Array.isArray((node = stack.pop()))) {
      for (var i = node.length; i--; ) {
        stack[stack.length] = node[i]
      }
    } else if (node != null && node !== true && node !== false) {
      if (typeof node === "number") {
        node = node + ""
      }
      children[children.length] = node
    }
  }

  return typeof tag === "string"
    ? {
        tag: tag,
        data: data || {},
        children: children
      }
    : tag(data, children)
}

[ngryman]

@pspeter3 Thanks for the writeup and the precisions.

What I like in h.ts is that with type definitions, it's self documented and brings type checking goodness for users (even if I still find this quite superfluous in a project like this). The fact that it catches bugs is not really an argument for me as a good test suite does very well the job (even way better) if well written of course.

What I don't like is that h.ts is verbose, constraints the user to use a predefined set of types (what if a future plugin wants to add a custom property to nodes or use different kind of node.data or whatever?). We'll end up with really complex type definitions to make things lax (like I saw in several other projects) totally defeating the purpose of self-documentation. I strongly prefer a clear and well written JSDoc, than generics everywhere.

h just needs that a node has a given shape, not a precise topology and type definitions. Using type definitions here makes me feel we are writing explicitly and loudly what's implicit or some times does not even matter. For an AST parser it would be another story though as there are a large variety of nodes and a spec to follow, Typescript would make more sense there IMO.

At the end of the day writing this in Typescript or vanilla JavaScript is only a matter of taste. Some developers like things to be explicitly written, others (me) prefer things to be KISS. It's like the old war between convention / configuration, nobody wins, nobody loses, you just choose what you feel comfortable with.

So @jbucaran, it's your call 👍

[jorgebucaran]

I'd like to keep this issue open and hope to get more feedback from other people.

[leo-shopify]

You could also use the jsdoc style type annotations along with --allowJs to keep the source files as standard js syntax. A very simple example to illustrate. I haven't used it yet so I don't know if it works as advertised or not.

[pspeter3]

Hmm, that actually might be the best option if it can generate DTS files.

[leo-shopify]

I don't think d.ts files can be extracted from .js files with jsdoc annotations.

[pspeter3]

Oh too bad.

[ngryman]

@leo-shopify Thanks for the link! I think it is mostly useful if you are mixing TypeScript / JS. In our case, if we only want type checking, other tools like Flow might a good option as they are dedicated to that particular job. What do you think?

@pspeter3 The more I think about this issue, the more I think you could provide a type declaration file. I know you mentioned you won't be interested in maintaining it (and I understand your point). But picodom has a very small API surface, and h signature is pretty much written in the stone. Most changed will be related to internal stuff. What do you think?

[leo-shopify]

@ngryman, yes, the gradual option is more beneficial in larger untyped code bases. If the trade offs between the two, from a developer's perspective, mostly come down to preference, I'd choose based on benefits to the users. It seems to me that typescript is evolving faster and is gaining larger adoption. I'd provably choose typescript today. Having said that, picodom is so small that even making the "wrong" choice doesn't seem that costly.

[pspeter3]

@ngryman I'd be happy to add the DTS file. Would you mind if I also added tests that assert the type definitions are correct?

[ngryman]

@pspeter3 I'm a total newbie to TypeScript so if you think it's worth, let's go.

/cc @jbucaran

[pspeter3]

Yeah, I think in order of DX it is:

• Correct type definitions
• No type definitions
• Incorrect type definitions

The tests will basically just assert that the type definitions have the right types.

[jorgebucaran]

@pspeter3 Cool! Let's do this. Hope it won't be as complex a HyperApp's typings. 😱:smirk::+1:

[jorgebucaran]

Closing in favor of #17.

[mindplay-dk]

In my opinion, there's a big advantage to using Typescript, in terms of code comprehension - type-hints really help with the understanding of the internals of the codebase; rather than increasing the learning curve, type-hints provide IDE support, making it easier to navigate and inspect the code.

While I understand that using Typescript might create a barrier for those who don't want to learn it, code with no inline documentation (mainly type-hints) can be really hard to absorb. I've attempted to read the 200 or so lines of code numerous times now, and still most of it is a black box to me.

The real advantage of using a small library such as this one, is not about whether the library is 1KB or 2KB - a typical network package is ~1500 bytes, and the practical difference between 1 or 2 network packages, when you have images etc. taking up hundred of packages during the same request, is rather moot in this day and age.

The important difference of using a minimalist library, is your ability to actually understand what you're using. Now, if most people don't get that, because the code is too difficult to read, we're not getting the full advantage of using a small library.

If you don't want to use Typescript, please consider using something else, like flow or js-doc to fully document the internals of the codebase.

Though, in my personal opinion, flow (and to some extent js-doc) come with about the same amount of learning curve in terms of syntax and tools - only with proprietary syntax hidden in doc-blocks, and less powerful IDE support. If the type-hints in Typescript are a distraction to some, they can read the output JS, which would be identical to what you have now - and with flow or js-doc, you'd have to "compile" the JS to validate and strip type-hints from the output, so the process and overhead is really much the same.

I'd also encourage you to use more idiomatic JS and avoid garbling the code to save bytes - working with "minified" code at design-time saves you literally almost no bandwidth in practice, and really gets in the way of comprehension, contributions, refactoring, etc.

You've done an absolutely tremendous job with this library, and I'm really excited to dig into this! :-)

But it would be so much more accesible if you would add inline type-hints and documentation, and unpack your "minified" statements to make the code more readable.

I'd love to help :-)

I could certainly help with the latter, making the code more idiomatic and readable, but I can't really help with the type-hinting, since I'd have real difficulty saying whether I've understood all of it correctly.

[jorgebucaran]

@mindplay-dk The important difference of using a minimalist library, is your ability to actually understand what you're using. Now, if most people don't get that, because the code is too difficult to read, we're not getting the full advantage of using a small library.

You hit the nail on the head. 💅🔨

That's the whole point of the 1 KB tagline. I am not saying my library downloads faster, and anyone who thought it was about that missed the forest for the trees.

If you don't want to use Typescript, please consider using something else, like flow or js-doc to fully document the internals of the codebase.

Yes, I know, but my focus has always been Hyperapp. I said no before, but I am not as strongly opinionated about Picodom as I am about Hyperapp, so come to think about it again, I could rewrite Picodom in TypeScript as a way to learn TypeScript! 🤔

The beauty of picodom is that it needs not worry about state management, making development much easier than Hyperapp.

[mindplay-dk]

I assumed you were very concerned about the package size based on your previous comment

Also, picodom is advertised as a 1 KB library and that is one of the project's main appeals. Do you know if rewriting everything in TypeScript could yield a bundle size >1 KB?

I took this as you being concerned that a valid Typescript version of the source would be slightly larger, because you might not be able to use some of the non-type-safe (non-idiomatic) shorthand expressions, e.g. assignments inside expressions, and x && y() rather than if (x) { y() }, etc. which some developers use as a means of saving bytes.

If your first concern is not about saving bytes, then I would be glad to help at least with unfolding some of the compressed code to simpler and more idiomatic code, if you'd like? That will likely make it much easier to port to Typescript or type-hinting with doc-blocks.

If your concern was that compiled Typescript code is somehow larger than plain JS, don't worry, it's not - of course the Typescript code has some longer lines due to the type-hints, but compared with doc-blocks, it's a lot shorter, and the compiled output (for code like this) is the same as the plain JS with no type-hints...

[jorgebucaran]

Yes!

[mindplay-dk]

@JorgeBucaran here's where I am with my Typescript rewrite:

https://bitbucket.org/mindplaydk/veedom/src

There's a bunch of noisy console output at this time, as I'm still evaluating some things by merely reading through the order of events, but a lot of tests are passing now, including tests for onremove and ondestroy - although I haven't tested for the loss of ordering yet, which may affect this implementation too.

This is very rough still - I'm just posting it in case you find some useful (or avoidable) ideas in the implementation or something :-)

[jorgebucaran]

Awesome! 👍

By the way, I'd like to combine onremove and ondestroy into a single event (without any functionality loss) like I did with hyperapp.

[mindplay-dk]

I'd like to combine onremove and ondestroy into a single event

Yeah, I saw that, did I comment? I don't understand how that would work.

If you have only one event that always gets fired, how do you know whether to trigger animation or clean-up when that event fires? It seems like, if you have only one event, and the same element needs to respond both to removal (with animation) and needs to clean something up, you're going to trigger unnecessary animation of elements when they actually get destroyed. I can't help but think you're over-simplifying here? Direct removal of a child-node is not the same thing as indirect removal of entire ranges of child-nodes. For example, if you have an animation for list items getting removed, but the app also has some function that toggles whether that list is visible at all. I don't understand how one event could be enough?

[jorgebucaran]

See here: https://github.com/hyperapp/hyperapp/blob/f9ad50ff8f89be0723ef1bdaf3350536dd7d8f11/src/index.js#L192

@mindplay-dk What do you mean? You either want to trigger an animation or clean-up resources and done should only be called on the top-most onremove, since we are not going to call removeChild for child elements, only for the parent (which is called last).

[mindplay-dk]

The last cycle of this test here - I expect the remove notification for the parent div only, but I expect destroy notifications for the parent and child divs.

If you have only one kind of notification, how can you tell if the element is being directly on indirectly removed? (or are you arguing you don't need to know?)

[jorgebucaran]

Currently (in hyperapp), onremove is only called for a top-level element and ondestroy is called for all the elements.

With a single onremove, if I am using onremove on a top-level element for animation, I will be calling done when the animation or transition is done. If am using it on a child/nested element, that means it's not for animation, but for cleaning up resources, so I don't need to call done.

And, of course, .removeChild will not be called for child elements (by default).

if (!isChild) parent.removeChild(element)

[mindplay-dk]

With a single onremove, if I am using onremove on a top-level element for animation, I will be calling done when the animation or transition is done. If am using it on a child/nested element, that means it's not for animation, but for cleaning up resources, so I don't need to call done.

How will you know which is the case?

For example:

<div class="app">
    <ul class="items">
        <li>...</li>
        <li>...</li>
        <li>...</li>
    </ul>
</div>

Let's say every <li> contains a date-picker control from a third-party library.

Every <li> is going to need an onremove handler for the animation that gets triggered for the individual items in the list. With a unified onremove, that's fine - I can clean up the date-picker instance when the animation finishes.

But every <li> is also going to need an ondestroy handler for the case where the entire <ul> gets toggled on and off. In this case, there's no animation. But we still need to clean up all the date-pickers. With a unified onremove, how are you able to tell the difference?

For the removal of list items, animation and then clean-up should be triggered - you've covered that case. For the removal of the entire list, only the clean-up should be triggered - that doesn't seem possible with only one life-cycle event.

[jorgebucaran]

@mindplay-dk I see what you are saying now, you are right! 🙃

Okay, what about this: pass a second argument to the function (element, isRoot) that tells you whether the element is being directly removed

(element, isRoot) => {
  element.datePicker.clean()
  if (isRoot) {
    return done => fadeout(element).then(done)
  }
}

[mindplay-dk]

pass a second argument to the function

I'd say that's a symptom of complecting two concerns into one.

The resulting complexity will spread and generate more if-statements and/or inline functions, making it harder to reason about these handlers - your example doesn't work, you're destroying the date-picker too soon: it'll pop out of view before the animation starts.

A working example might look more like this:

(element, isRoot) => {
  if (isRoot) {
    return done => fadeout(element).then(() => {
      element.datePicker.clean()
      done()
    })
  } else {
    element.datePicker.clean()
  }
}

If the clean-up logic is more than a single line/statement, you'll likely need an inline function too:

(element, isRoot) => {
  if (isRoot) {
    return done => fadeout(element).then(() => {
      cleanup()
      done()
    })
  } else {
    cleanup()
  }

  function cleanup() {
    element.datePicker.clean()
    // ... whatever else
  }
}

In my opinion, this is a clear case of conflating multiple concerns - while a lower number of life-cycle events may seem "simpler", this is somewhat of a deception, as that one life-cycle event has to support all the use-cases that could have been handled independently with two events.

Why the resistance to separating two concerns that can be cleanly separated?

[mindplay-dk]

Meanwhile, back in the jungle: I added the order preservation test for deferred removals to my implementation, and, it suffers from exactly the same problem - I'm stuck with the exact same dead ends as with picodom, and still no closer to solving it either for picodom or my own patcher 🙄

[jorgebucaran]

So, something like this is the cleaner alternative in your opinion.

function cleanup(element) {
  element.datePicker.clean()
  // ... whatever else
}

onremove: element => done =>
  fadeout(element).then(() => {
    cleanup(element)
    done()
  })

ondestroy: element => {
  cleanup(element)
}

[jorgebucaran]

And for comparison, here is mine.

function cleanup(element) {
  element.datePicker.clean()
  // ... whatever else
}

onremove: (element, isRoot) =>
  isRoot
    ? done =>
        fadeout(element).then(() => {
          cleanup(element)
          done()
        })
    : cleanup(element)

[jorgebucaran]

Or this

function cleanup(element, done) {
  element.datePicker.clean()
  // ... whatever else
  if (done) done()
}

onremove: (element, isRoot) =>
  isRoot
    ? done => fadeout(element).then(() => cleanup(element, done))
    : cleanup(element)

[mindplay-dk]

No, just this.

onremove: element => done =>
  fadeout(element).then(done)

ondestroy: element => {
  element.datePicker.clean()
  // ... whatever else
}

Pretty clean and simple? :-)