Use struct NodeId(u64) instead of slotmap::DefaultKey in all public APIs

[nicoburns]

Objective

It's not currenty possible to use the LayoutTree trait without using a SlotMap as the backing storage because it uses slotmap::DefaultKey in the API! This replaces that with u64.

Fixes #327

Notes

Benchmark result show this as perf neutral even though a small amount of work is introduced by converting the DefaultKey (backed by two u32s) into a u64.

Possible alternatives

We could make all of our code generic over the key type like in #326. However this introduces generics all over the place affecting both code readability and compile times. I figure that everyone is likely to have a u64 compatible key, and if someone doesn't then we can revisit this issue at a later date.

[TimJentzsch]

I think I would prefer to have an explicit newtype for this (e.g. NodeId) that wraps u64 and provides From conversion methods.

That would make the code easier to read (effectively adding more documentation) and would make it easier to change the internal representation later if needed.

[nicoburns]

I think I would prefer to have an explicit newtype for this (e.g. NodeId) that wraps u64 and provides From conversion methods.

Hmm... so something like:

struct NodeId(u64)

I can see advantages of this (for one I could implement conversion function to and from slotmap keys, which would reduce a bunch of boilerplate). Would you want that type exposed externally in the LayoutTree trait instead of u64, or would it just be for internal use?

[TimJentzsch]

I think I would prefer to have an explicit newtype for this (e.g. NodeId) that wraps u64 and provides From conversion methods.

Hmm... so something like:

struct NodeId(u64)

Yup, that's exactly what I had in mind :)

I can see advantages of this (for one I could implement conversion function to and from slotmap keys, which would reduce a bunch of boilerplate). Would you want that type exposed externally in the LayoutTree trait instead of u64, or would it just be for internal use?

I think it would be reasonable to export it as well, but I don't have strong opinions on this

[Weibye]

The change itself looks good, I do agree with @TimJentzsch about the newtype id.

[nicoburns]

Ok, so I've introduced a NodeId newtype as suggested. And I've gone ahead and replaced all public uses of type taffy::node::Node = slotmap::DefaultKey with it (and then deleted taffy::node::Node). This is super nice as it makes slotmap an implementation detail that is no longer part of the public API at all.

NodeId has From/Into conversions defined for u64, usize (which are intended for external interop), and slotmap::DefaultKey (which we use internally).

[TimJentzsch]

LGTM.

I also like the naming of NodeId more than the previous Node, which indicated that there is some data stored in Node.
This was a source of confusion for me before.

[TimJentzsch]

Do you want to add this to the release notes in this PR or in a future one?

[nicoburns]

Do you want to add this to the release notes in this PR or in a future one?

Release notes added :)

[nicoburns]

Performance seems to be roughly neutral :) Merging!