Add shared-internals package

[marvinhagemeister]

As discussed in our monthly meeting, here is the package with the internal getter and setter methods intended for debugging/testing enhancements. For this I combined all the mangled names I could find in enzyme-adapter-preact-pure and our (yet to be released) preact-devtools.

[coveralls]

Coverage decreased (-1.3%) to 96.331% when pulling d2dcbc2 on shared-internals into 5d1dc19 on master.

[JoviDeCroock]

This is in line with how I was thinking of it, nice work Marvin! 💯

[cristianbote]

I've used this in the past https://github.com/duxiaofeng-github/preact-responder-event-plugin/blob/master/src/index.ts#L8 maybe we could add it? What do you think?

[marvinhagemeister]

@cristianbote Good point, added those 👍

[marvinhagemeister]

Alright, made a few changes and I think now it's ready for another review round. These are the changes:

• Add tests
• Remove getter for _lastDomChild because we only use it inside diffChildren and reset it immediately to null. So the getter would always return null in every scenario.

[cristianbote]

🎉 awesome job, Marvin! This will help a lot!

[robertknight]

I created preactjs/enzyme-adapter-preact-pure#86 to test this locally. It looks great except that the test adapter is still missing a function to get the root vnode rendered into a given DOM node. This is the _children property set on DOM container nodes.

[robertknight]

By "public types" do you mean "public types for the arguments". Are they different from "private types"?

[marvinhagemeister]

It's referring to our public Options type. It doesn't have the internal _commit/_root/_diff properties in the interface. Extending the existing interface would be incorrect as our private property names are mangled.

[andrewiggins]

Hmm is little nervous exposing this option before we do 2 phase render. By exposing the option we also expose it's contract (i.e. arguments) and I suspect the commit option will change after 2 phase (personally think it might be interesting to rewrite commit option to enable different renders).

Which libraries is this need for? Devtools? I also really want the devtools lol

[marvinhagemeister]

That's a very good point! For now the devtools "bridge" will likely live in preact/debug so there is no immediate need for the devtools expose this. We do have one known outside consumer though: https://github.com/duxiaofeng-github/preact-responder-event-plugin/blob/master/src/index.ts#L8 . Same is true for the Composition API in #1923 .

I'm wondering if a 2-phase renderer would be released as semver major or minor. It shouldn't be a visible breaking change for users from an API point of view though.

I'm admittedly a bit torn. On the one hand I'm ok with doing breaking changes here for our shared internals, but on the other hand we likely will have users depend on it at some point.

[marvinhagemeister]

@andrewiggins The performance tests are causing the build failures. I'm wondering if the issue is the way they are run on Travis. Our suite assumes that they'll be executed under the same conditions but maybe we get a different machine/vm on each run with different specs?

[marvinhagemeister]

FYI: We disabled running our benchmark tests on our CI with #2167.

[JoviDeCroock]

Let's merge this all right?

[developit]

Apologies for being super late to review this one. A thought on the hook abstraction: what if we abstracted the calling of old hooks? Something like an Express middleware:

function empty() {}

export function installCommitHook(options, hookFn) {
  const old = options._commit || empty;
  // we only have hooks with up to 2 arguments
  options._commit = (a, b) => hookFn(old, a, b);
}

This would allow hooks to decide when to call their originals, like before and after middleware:

installCommitHook(options, (next, vnode) => {
  // run any previous commit hook:
  next(vnode);
  vnode.foo = 'bar';
});